Skip to content

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

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

katcharov
Copy link
Collaborator

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).

@katcharov katcharov requested a review from vbabanin June 11, 2024 17:51
* 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()) {
Copy link
Collaborator Author

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).

Copy link
Collaborator Author

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).

@katcharov katcharov force-pushed the JAVA-5379-csot branch 2 times, most recently from b71c164 to bf55f6a Compare June 11, 2024 18:10
@katcharov katcharov requested a review from vbabanin June 14, 2024 14:43
@@ -439,30 +397,6 @@ void advanceOrThrowTransformAfterFirstAttempt(final TimeoutContext timeoutContex
}));
}

@Test
void advanceOrThrowTransformThrowsTimeoutExceptionAfterFirstAttempt() {
Copy link
Member

@vbabanin vbabanin Jun 14, 2024

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.

Copy link
Member

@vbabanin vbabanin Jun 14, 2024

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.

@katcharov katcharov merged commit 28c6df9 into mongodb:CSOT Jun 14, 2024
2 of 3 checks passed
@katcharov katcharov deleted the JAVA-5379-csot branch June 14, 2024 22:51
@vbabanin vbabanin mentioned this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants