Remove hasExpired and expiry checks in retries #1418
Conversation
driver-core/src/test/unit/com/mongodb/internal/async/function/RetryStateTest.java
Outdated
Show resolved
Hide resolved
| * The function of isLastIteration() is to indicate if retrying has been explicitly halted. Such a stop is not interpreted as | ||
| * a timeout exception but as a deliberate cessation of retry attempts. | ||
| */ | ||
| if (hasTimeoutMs() && !loopState.isLastIteration()) { |
There was a problem hiding this comment.
The logic for removing these is that it is safe to behave as if a timeout has not expired (even if it has), because it will be detected and thrown by an ensuing blocking operation.
This relies on the assumption that all blocking operations will throw when given an expired timeout (this should be the case, because all blocking operations should now be wrapped in Timeout's branching "call" methods).
There was a problem hiding this comment.
Tests were failing. The logic related to timeout exceptions in RetryState is necessary, but complicated to extract. I've restored it, but the isExpired check is still removed (so this still relies on the ensuing blocking operation throwing a timeout exception).
katcharov
force-pushed
the
JAVA-5379-csot
branch
2 times, most recently
from
b71c164 to
bf55f6a
Compare
katcharov
force-pushed
the
JAVA-5379-csot
branch
from
bf55f6a to
8c0febd
Compare
driver-core/src/main/com/mongodb/internal/async/function/RetryState.java
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java
Outdated
Show resolved
Hide resolved
| @@ -439,30 +397,6 @@ void advanceOrThrowTransformAfterFirstAttempt(final TimeoutContext timeoutContex | |||
| })); | |||
| } | |||
|
|
|||
| @Test | |||
| void advanceOrThrowTransformThrowsTimeoutExceptionAfterFirstAttempt() { | |||
There was a problem hiding this comment.
This test asserts the behavior of error transformation along with timeout handling. Despite the removal of timeout checks within the RetryState, we should ensure that the logic related to error transformation is still tested.
The same applies to advanceOrThrowPredicateThrowsTimeoutAfterFirstAttempt.
There was a problem hiding this comment.
As discussed on the call with @katcharov, those tests will be ignored and updated later.
JAVA-5379
This removes
hasExpired, and expiry checks in retries (retries rely exclusively on timeout exceptions being thrown by blocking operations, to signal that the retry should halt).