-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Remove hasExpired and expiry checks in retries #1418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
b71c164
to
bf55f6a
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).