Make RetryState.isLastAttempt private, and simplify the code#1961
Make RetryState.isLastAttempt private, and simplify the code#1961stIncMale wants to merge 4 commits intomongodb:mainfrom
RetryState.isLastAttempt private, and simplify the code#1961Conversation
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private boolean isLastAttempt(final Throwable attemptException) { | ||
| boolean lastIteration = loopState.isLastIteration(); | ||
| boolean operationTimeout = retryUntilTimeoutThrowsException && attemptException instanceof MongoOperationTimeoutException; | ||
| assertFalse(lastIteration && operationTimeout); |
There was a problem hiding this comment.
breakAndThrowIfRetryAnd/breakAndCompleteIfRetryAnd can't cause both lastIteration and operationTimeout to be true. Therefore, technically, only markAsLastAttempt is capable of that. The latter is called only from MixedBulkWriteOperation, as the AI correctly identified, and only if MongoException happens. So it looks like lastIteration && operationTimeout may indeed happen. However, if it happens, then the iteration is marked as the last one because of the exception. Therefore, we should remove assertFalse(lastIteration && operationTimeout), but nothing else needs to be changed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…imit does not conflict with timeout
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
With the backpressure spec changes, it has become painfully obvious that the logic of restricting the number of retries based on either a fixed number of retries, or a CSOT exception, is business logic, and not the internal logic of
RetryingSupplier/RetryState. Given that most of the business logic lives outside ofRetryingSupplier/RetryState, the part about the limit will also be moved out. This PR, just like #1952, does the preliminary work.JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141