Adds overload retry for not retryable commands - #2048
Draft
vbabanin wants to merge 1 commit into
Draft
Conversation
…d/write retry policies - Implements overload-only retry (gated on retryWrites, per client-backpressure) for the write commands that previously dispatched with no retry wrapper: createIndexes, dropIndexes, create/drop/rename collection, aggregate with $out/$merge, and the search-index commands. - Removes the JAVA-5956 skips that hid these commands from the unified client-backpressure suite, and adds prose tests (not part of the spec suite) covering the commands that have no unified test coverage. JAVA-6308
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Constructor compatibility, legacy integration, and multi-command retry-scope defects must be resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds retryWrites-gated overload retries to previously non-retryable write commands across core and synchronous driver paths.
Changes:
- Adds synchronous and asynchronous overload retry loops for DDL and search-index operations.
- Propagates retry settings to write aggregations.
- Enables unified backpressure tests and adds prose coverage.
Human review is required. Blocking issues include stale constructor call sites, incomplete legacy-driver wiring, and incorrect retry scopes for encrypted collection operations.
File summaries
| File | Description |
|---|---|
UnifiedTestModifications.java |
Re-enables backpressure tests. |
BackpressureProseTest.java |
Adds command retry coverage. |
MongoDatabaseImpl.java |
Propagates aggregate retryWrites. |
MongoCollectionImpl.java |
Propagates aggregate retryWrites. |
AggregateIterableImpl.java |
Accepts write retry settings. |
UpdateSearchIndexesOperation.java |
Adds retry configuration. |
RenameCollectionOperation.java |
Adds overload retry loop. |
Operations.java |
Wires retry settings into operations. |
DropSearchIndexOperation.java |
Adds retry configuration. |
DropIndexOperation.java |
Adds overload retry loop. |
DropDatabaseOperation.java |
Adds overload retry loop. |
DropCollectionOperation.java |
Adds overload retry loop. |
CreateViewOperation.java |
Adds overload retry loop. |
CreateSearchIndexesOperation.java |
Adds retry configuration. |
CreateIndexesOperation.java |
Adds overload retry loop. |
CreateCollectionOperation.java |
Adds overload retry loop. |
AggregateToCollectionOperation.java |
Enables write-gated overload retries. |
AbstractWriteSearchIndexOperation.java |
Centralizes search-index retries. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| final ReadConcern readConcern, final WriteConcern writeConcern, final OperationExecutor executor, | ||
| final List<? extends Bson> pipeline, final AggregationLevel aggregationLevel, | ||
| final boolean retryReads, @Nullable final Integer maxAdaptiveRetriesSetting, | ||
| final boolean retryWrites, final boolean retryReads, @Nullable final Integer maxAdaptiveRetriesSetting, |
Comment on lines
+271
to
+275
| Supplier<Void> retryingCommandExecutor = decorateWithRetries(retryControl, operationContext, () -> { | ||
| retryControl.getPolicy().onCommand(this::getCommandName); | ||
| return withConnection(binding, operationContext, (connection, operationContextWithMinRtt) -> { | ||
| checkEncryptedFieldsSupported(connection.getDescription()); | ||
| getCommandFunctions().forEach(commandCreator -> |
Comment on lines
+150
to
+152
| AsyncCallbackSupplier<Void> retryingCommandExecutor = decorateWithRetriesAsync(retryControl, operationContext, supplierCallback -> { | ||
| SingleResultCallback<Void> errHandlingCallback = errorHandlingCallback(supplierCallback, LOGGER); | ||
| getEncryptedFields((AsyncReadWriteBinding) binding, operationContext, (result, t) -> { |
Comment on lines
622
to
+623
| CreateCollectionOperation operation = new CreateCollectionOperation( | ||
| assertNotNull(namespace).getDatabaseName(), collectionName, writeConcern) | ||
| assertNotNull(namespace).getDatabaseName(), collectionName, writeConcern, isRetryWrites(), maxAdaptiveRetriesSetting) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds overload-only retry support to 11 write-command operations that previously dispatched commands without any retry wrapper.
The retry policy is gated on
retryWritesand uses:includeOverload(maxAdaptiveRetriesSetting, ErrorPropagation.AS_WRITE_POLICY)It intentionally does not use
includeWrite(), because these commands are not retryable writes under the retryable-writes spec.Commands covered
createIndexesCreateIndexesOperationdropIndexesDropIndexOperationcreateCreateCollectionOperationcreateviewCreateViewOperationdropDropCollectionOperationdropDatabaseDropDatabaseOperationrenameCollectionRenameCollectionOperationaggregatewith$out/$mergeAggregateToCollectionOperationcreateSearchIndexesCreateSearchIndexesOperationupdateSearchIndexUpdateSearchIndexesOperationdropSearchIndexDropSearchIndexOperationImplementation details
createSpecRetryControl+decorateWithRetries.createSpecRetryControl+decorateWithRetriesAsync.onCommand(this::getCommandName)for retry debug logging.retryWritesandmaxAdaptiveRetriesSettingfrom the client layer throughOperationsinto the affected operations.$out/$mergerespects the actualretryWritessetting instead of always usingtrue. (//TODO check this)Scope notes
Already covered before this change:
updateMany/deleteMany/bulkWritefindAndModifyrunCommandgetMoreexecuteRetryableReadIntentionally excluded:
mapReduce{w:0}writeskillCursorsTest coverage
JAVA-5956skips for unified client-backpressure tests:createIndexdropIndexdropIndexes