Refactoring for backpressure#1952
Conversation
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
…peration` JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
- Document the `doWithRetriesDisabled*` methods, leave `TODO-JAVA-5956` to fix them. - Add `ClientBulkWriteCommandOkResponse`, use it instead of a generic `BsonDocument`. - Naming of callbacks. - Code formatting. JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
…rite`/`canRetryRead` JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
There was a problem hiding this comment.
Pull request overview
Refactors internal retry/backpressure-related helpers to simplify retry eligibility checks, rename/clarify retry attachments, and improve retry logging across sync/async operations and bulk write flows.
Changes:
- Rename and clarify retry-related attachments and logging (
retryableWriteCommandFlag,logRetryCommand, logging retry predicates). - Simplify retry eligibility checks by removing unused parameters (e.g.,
canRetryReadno longer depends onServerDescription). - Refactor bulk write cursor exhaust + retry-disabling scaffolding in
ClientBulkWriteOperation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| driver-core/src/test/unit/com/mongodb/internal/operation/OperationHelperSpecification.groovy | Updates unit test to match new canRetryRead signature and removes unused server-description fixtures. |
| driver-core/src/main/com/mongodb/internal/operation/retry/AttachmentKeys.java | Renames retry flag attachment key and removes unused bulk write result attachment. |
| driver-core/src/main/com/mongodb/internal/operation/SyncOperationHelper.java | Switches to new retry logging helpers, updates retryable read/write checks, and signature/formatting tweaks. |
| driver-core/src/main/com/mongodb/internal/operation/OperationHelper.java | Simplifies retry helper signatures and updates internal exception Javadoc references. |
| driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java | Aligns retry logging rename and removes unused parameters from batch execution helpers. |
| driver-core/src/main/com/mongodb/internal/operation/ListIndexesOperation.java | Updates retryable read checks to use new canRetryRead(OperationContext) signature. |
| driver-core/src/main/com/mongodb/internal/operation/ListCollectionsOperation.java | Updates retryable read checks to use new canRetryRead(OperationContext) signature. |
| driver-core/src/main/com/mongodb/internal/operation/FindOperation.java | Updates retryable read checks to use new canRetryRead(OperationContext) signature. |
| driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java | Renames retry logging helpers and refactors retry decision helpers/labels. |
| driver-core/src/main/com/mongodb/internal/operation/ClientBulkWriteOperation.java | Refactors bulk write OK response handling, cursor exhaust retry-disabling helpers, and callback naming. |
| driver-core/src/main/com/mongodb/internal/operation/AsyncOperationHelper.java | Switches to new retry logging helpers and updates retryable read/write checks. |
| driver-core/src/main/com/mongodb/internal/async/function/RetryState.java | Minor Javadoc cleanup and wording tweak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final int batchSize, final Decoder<T> decoder, | ||
| @Nullable final BsonValue comment, final ConnectionSource source, | ||
| final Connection connection, final OperationContext operationContext) { | ||
| static <T> BatchCursor<T> cursorDocumentToBatchCursor( |
There was a problem hiding this comment.
AsyncOperationHelper.cursorDocumentToAsyncBatchCursor returns AsyncBatchCursor, not AsyncCommandBatchCursor. I changed SyncOperationHelper.cursorDocumentToBatchCursor to return the corresponding sync type.
| static boolean shouldAttemptToRetryWriteAndAddRetryableLabel(final RetryState retryState, final Throwable attemptFailure) { | ||
| return getAttemptFailureNotToRetryOrAddRetryableLabel(retryState, attemptFailure) != null; | ||
| } |
There was a problem hiding this comment.
The != check here was incorrect, but fortunately, it hasn't been used. The problem was fund by Copilot.
|
The evergreen failures seem to match those for |
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I strongly recommend reviewing commits one by one.
While the refactoring is done as part of me working on the backpressure project, all the changes make sense as is on
master. And given that I have to locally work onmasterfor now anyway, it makes sense for this PR to go tomaster.AI usage: all the changes were made manually and reviewed locally using Claude Code.
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141