CSHARP-5723: Add exponential backoff to operation retry loop for server overloaded errors#1859
Open
papafe wants to merge 2 commits intomongodb:mainfrom
Open
CSHARP-5723: Add exponential backoff to operation retry loop for server overloaded errors#1859papafe wants to merge 2 commits intomongodb:mainfrom
papafe wants to merge 2 commits intomongodb:mainfrom
Conversation
papafe
force-pushed
the
csharp5723-exponential_backoff
branch
from
3ae9bbf to
494c2b2
Compare
papafe
force-pushed
the
csharp5723-exponential_backoff
branch
from
61061b0 to
d0f56f4
Compare
papafe
force-pushed
the
csharp5723-exponential_backoff
branch
from
fc1e7b4 to
1d7fe34
Compare
papafe
force-pushed
the
csharp5723-exponential_backoff
branch
from
4e96e11 to
a8bf6c5
Compare
papafe
force-pushed
the
csharp5723-exponential_backoff
branch
from
d8e18e6 to
35811c5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds client backpressure/adaptive retry support to the C# driver by introducing a token-bucket based retry limiter and wiring retryability flags through read/write operations, alongside new/updated spec tests and connection-string support.
Changes:
- Introduce
adaptiveRetriesconnection-string/client setting and propagate it into cluster creation (token bucket presence). - Add/propagate
CanBeRetriedthrough many operations/cursors and update server/cluster plumbing to carry aTokenBucket. - Update handshake/hello construction and add/adjust unified/spec/integration tests for backpressure behaviors.
Reviewed changes
Copilot reviewed 151 out of 153 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/TokenBucketTests.cs | Adds unit tests for new token bucket behavior. |
| tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs | Adds unified spec coverage entry point for client backpressure tests and refactors skip helper. |
| tests/MongoDB.Driver.Tests/Specifications/server-selection/InWindowTestRunner.cs | Updates mock server factory signature to accept token bucket. |
| tests/MongoDB.Driver.Tests/Specifications/server-discovery-and-monitoring/prose-tests/ServerDiscoveryProseTests.cs | Ensures test collection is cleared before insert (stabilizes test). |
| tests/MongoDB.Driver.Tests/Specifications/mongodb-handshake/prose-tests/MongoDbHandshakeProseTests.cs | Adjusts namespace/category and adds a new handshake prose test for backpressure. |
| tests/MongoDB.Driver.Tests/Specifications/connection-string/ConnectionStringTestRunner.cs | Adds adaptiveRetries assertion handling in connection string spec runner. |
| tests/MongoDB.Driver.Tests/MongoUrlTests.cs | Adds AdaptiveRetries to MongoUrl round-trip tests. |
| tests/MongoDB.Driver.Tests/MongoUrlBuilderTests.cs | Adds AdaptiveRetries to builder tests and new parsing theory. |
| tests/MongoDB.Driver.Tests/MongoClientSettingsTests.cs | Adds settings tests for AdaptiveRetries and clone/equality/defaults. |
| tests/MongoDB.Driver.Tests/GridFS/GridFSBucketTests.cs | Ensures mocked client exposes Settings (needed for new retryability wiring). |
| tests/MongoDB.Driver.Tests/Core/Servers/ServerMonitorTests.cs | Updates expected hello documents to include backpressure. |
| tests/MongoDB.Driver.Tests/Core/Servers/ServerFactoryTests.cs | Updates server factory tests for new token bucket parameter. |
| tests/MongoDB.Driver.Tests/Core/Servers/RoundTripTimeMonitorTests.cs | Updates expected hello documents to include backpressure. |
| tests/MongoDB.Driver.Tests/Core/Servers/LoadBalancedServerTests.cs | Updates load-balanced server construction to pass token bucket. |
| tests/MongoDB.Driver.Tests/Core/Operations/RetryableWriteOperationExecutorTests.cs | Updates retryable write context construction with canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Operations/RetryableReadOperationExecutorTests.cs | Temporarily disables retry decision tests (needs restoration). |
| tests/MongoDB.Driver.Tests/Core/Operations/RenameCollectionOperationTests.cs | Updates CreateCommand signature usage (adds transaction number arg). |
| tests/MongoDB.Driver.Tests/Core/Operations/ReadCommandOperationTests.cs | Ensures read binding mock provides a token bucket. |
| tests/MongoDB.Driver.Tests/Core/Operations/EndTransactionOperationTests.cs | Updates reflection helper for new CreateCommand signature. |
| tests/MongoDB.Driver.Tests/Core/Operations/DropIndexOperationTests.cs | Updates CreateCommand signature usage (connection description + transaction number). |
| tests/MongoDB.Driver.Tests/Core/Operations/DropDatabaseOperationTests.cs | Updates CreateCommand signature usage (connection description + transaction number). |
| tests/MongoDB.Driver.Tests/Core/Operations/DropCollectionOperationTests.cs | Updates CreateCommand signature usage (connection description + transaction number). |
| tests/MongoDB.Driver.Tests/Core/Operations/CursorTests.cs | Updates cursor ctor signature to include maxTime and canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Operations/CreateViewOperationTests.cs | Updates CreateCommand signature usage; file encoding/header changed. |
| tests/MongoDB.Driver.Tests/Core/Operations/CreateIndexesOperationTests.cs | Updates CreateCommand signature usage (adds transaction number arg). |
| tests/MongoDB.Driver.Tests/Core/Operations/CreateCollectionOperationTests.cs | Updates CreateCommand signature usage (connection description + transaction number). |
| tests/MongoDB.Driver.Tests/Core/Operations/AsyncCursorTests.cs | Updates cursor ctor signature; disables a getMore/session test (needs re-enable). |
| tests/MongoDB.Driver.Tests/Core/Operations/AsyncCursorSourceEnumeratorTests.cs | Updates cursor ctor signature to include canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Operations/AsyncCursorEnumeratorTests.cs | Updates cursor ctor signature to include maxTime and canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Operations/AggregateToCollectionOperationTests.cs | Updates CreateCommand signature usage (adds transaction number arg). |
| tests/MongoDB.Driver.Tests/Core/LoadBalancingIntegrationTests.cs | Updates retryable context construction with canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Jira/CSharp3302Tests.cs | Updates mock server factory signature and provides token bucket on server mock. |
| tests/MongoDB.Driver.Tests/Core/IAsyncCursorSourceExtensionsTests.cs | Updates cursor ctor signature to include canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/IAsyncCursorExtensionsTests.cs | Updates cursor ctor signature to include canBeRetried. |
| tests/MongoDB.Driver.Tests/Core/Connections/HelloHelperTests.cs | Updates hello expectations to include backpressure. |
| tests/MongoDB.Driver.Tests/Core/Configuration/ConnectionStringTests.cs | Adds tests for parsing adaptiveRetries. |
| tests/MongoDB.Driver.Tests/Core/Clusters/LoadBalancedClusterTests.cs | Adds assertions about token bucket presence based on adaptive retries. |
| tests/MongoDB.Driver.Tests/Core/Clusters/ClusterTests.cs | Adds assertions about token bucket presence based on adaptive retries; updates factory signature. |
| tests/MongoDB.Driver.Tests/Core/Bindings/ChannelSourceReadWriteBindingTests.cs | Removes tests for deleted binding type. |
| tests/MongoDB.Driver.Tests/ClusterRegistryTests.cs | Updates ClusterKey construction and asserts token bucket presence. |
| tests/MongoDB.Driver.Tests/ClusterKeyTests.cs | Adds AdaptiveRetries to cluster key equality coverage. |
| tests/MongoDB.Driver.Tests/ClientDocumentHelperTests.cs | Removes unused Moq import. |
| tests/MongoDB.Driver.TestHelpers/Core/MockClusterableServerFactory.cs | Updates mock factory signature and passes token bucket into servers. |
| tests/MongoDB.Driver.TestHelpers/Core/CoreExceptionHelper.cs | Adds helper to create command exceptions with multiple labels. |
| src/MongoDB.Driver/TokenBucket.cs | Introduces internal token bucket used to limit backpressure retries. |
| src/MongoDB.Driver/MongoUrlBuilder.cs | Adds AdaptiveRetries builder property, parsing, and serialization. |
| src/MongoDB.Driver/MongoUrl.cs | Adds AdaptiveRetries property on parsed URLs. |
| src/MongoDB.Driver/MongoDatabase.cs | Propagates CanBeRetried onto operations created by database APIs. |
| src/MongoDB.Driver/MongoClientSettings.cs | Adds AdaptiveRetries setting, cloning/equality/hash/toString, and FromUrl wiring. |
| src/MongoDB.Driver/MongoClient.cs | Propagates CanBeRetried onto client-created operations. |
| src/MongoDB.Driver/GridFS/GridFSSeekableDownloadStream.cs | Wires CanBeRetried into chunk find operation; exposes RetryReads. |
| src/MongoDB.Driver/GridFS/GridFSForwardOnlyUploadStream.cs | Wires retry flags into abort bulk write. |
| src/MongoDB.Driver/GridFS/GridFSForwardOnlyDownloadStream.cs | Wires CanBeRetried into chunk find operations. |
| src/MongoDB.Driver/GridFS/GridFSBucket.cs | Propagates retry flags into many GridFS operations and adds notes about download stream retry wiring. |
| src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs | Simplifies “message was sent” handling with an early return guard. |
| src/MongoDB.Driver/Core/Servers/ServerFactory.cs | Updates factory interface/signature to pass token bucket into servers. |
| src/MongoDB.Driver/Core/Servers/Server.cs | Stores/exposes token bucket on servers. |
| src/MongoDB.Driver/Core/Servers/SelectedServer.cs | Exposes token bucket from selected server wrapper. |
| src/MongoDB.Driver/Core/Servers/LoadBalancedServer.cs | Passes token bucket into base server. |
| src/MongoDB.Driver/Core/Servers/IServer.cs | Adds token bucket to server interface. |
| src/MongoDB.Driver/Core/Servers/IClusterableServerFactory.cs | Adds token bucket parameter to factory interface. |
| src/MongoDB.Driver/Core/Servers/DefaultServer.cs | Passes token bucket into base server. |
| src/MongoDB.Driver/Core/Operations/UpdateSearchIndexOperation.cs | Makes operation retryable-write aware and uses retryable executor paths. |
| src/MongoDB.Driver/Core/Operations/RetryableWriteContext.cs | Adds CanBeRetried, random, and optional secondary criteria plumbing. |
| src/MongoDB.Driver/Core/Operations/RetryableWriteCommandOperationBase.cs | Adds CanBeRetried and adjusts command creation signature. |
| src/MongoDB.Driver/Core/Operations/RetryableUpdateCommandOperation.cs | Updates command creation signature and payload creation. |
| src/MongoDB.Driver/Core/Operations/RetryableReadContext.cs | Adds CanBeRetried and random to read context. |
| src/MongoDB.Driver/Core/Operations/RetryableInsertCommandOperation.cs | Updates command creation signature. |
| src/MongoDB.Driver/Core/Operations/RetryableDeleteCommandOperation.cs | Updates command creation signature. |
| src/MongoDB.Driver/Core/Operations/RetryabilityHelper.cs | Adds backpressure retry helpers, label checks, and backoff constants. |
| src/MongoDB.Driver/Core/Operations/RenameCollectionOperation.cs | Makes rename collection retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/ReadCommandOperation.cs | Adds CanBeRetried and passes into retryable read context. |
| src/MongoDB.Driver/Core/Operations/OperationExtensionMethods.cs | Removes extension helpers that depended on removed binding type. |
| src/MongoDB.Driver/Core/Operations/MapReduceOutputToCollectionOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/MapReduceOperationBase.cs | Adds optional transaction number parameter to command creation. |
| src/MongoDB.Driver/Core/Operations/MapReduceOperation.cs | Adds CanBeRetried and passes into read command operation. |
| src/MongoDB.Driver/Core/Operations/ListIndexesUsingCommandOperation.cs | Adds CanBeRetried and passes into contexts/cursors. |
| src/MongoDB.Driver/Core/Operations/ListIndexesOperation.cs | Adds CanBeRetried and passes into context/operation. |
| src/MongoDB.Driver/Core/Operations/ListDatabasesOperation.cs | Adds CanBeRetried and passes into underlying command op. |
| src/MongoDB.Driver/Core/Operations/ListCollectionsOperation.cs | Adds CanBeRetried and passes into contexts/cursors. |
| src/MongoDB.Driver/Core/Operations/IRetryableOperation.cs | Adds CanBeRetried to retryable op interfaces. |
| src/MongoDB.Driver/Core/Operations/FindOperation.cs | Adds CanBeRetried and passes into contexts/cursors/command op. |
| src/MongoDB.Driver/Core/Operations/FindAndModifyOperationBase.cs | Adds CanBeRetried and passes into write retry executor. |
| src/MongoDB.Driver/Core/Operations/EstimatedDocumentCountOperation.cs | Adds CanBeRetried and passes into read context. |
| src/MongoDB.Driver/Core/Operations/DropSearchIndexOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/DropIndexOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/DropDatabaseOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/DropCollectionOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/DistinctOperation.cs | Adds CanBeRetried and passes into read context/command op. |
| src/MongoDB.Driver/Core/Operations/CreateViewOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/CreateSearchIndexesOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/CreateIndexesOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/CreateCollectionOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/CountOperation.cs | Adds CanBeRetried and passes into read context/command op. |
| src/MongoDB.Driver/Core/Operations/CountDocumentsOperation.cs | Propagates CanBeRetried into aggregate operation creation. |
| src/MongoDB.Driver/Core/Operations/ClientBulkWriteOperation.cs | Updates retryable write context creation and cursor ctor signature. |
| src/MongoDB.Driver/Core/Operations/ChangeStreamOperation.cs | Adds CanBeRetried and passes into read contexts. |
| src/MongoDB.Driver/Core/Operations/BulkUnmixedWriteOperationBase.cs | Adds CanBeRetried and passes into write context. |
| src/MongoDB.Driver/Core/Operations/BulkMixedWriteOperation.cs | Adds CanBeRetried and passes into write context. |
| src/MongoDB.Driver/Core/Operations/AsyncCursor.cs | Adds canBeRetried and uses ReadCommandOperation for getMore. |
| src/MongoDB.Driver/Core/Operations/AggregateToCollectionOperation.cs | Makes operation retryable-write aware and refactors execution. |
| src/MongoDB.Driver/Core/Operations/AggregateOperation.cs | Adds CanBeRetried and passes into contexts/cursors/command op. |
| src/MongoDB.Driver/Core/Connections/HelloHelper.cs | Adds backpressure field into hello command construction. |
| src/MongoDB.Driver/Core/Configuration/ConnectionString.cs | Adds parsing/storage for adaptiveRetries. |
| src/MongoDB.Driver/Core/Configuration/ClusterBuilder.cs | Plumbs adaptive retries into cluster factory creation. |
| src/MongoDB.Driver/Core/Clusters/SingleServerCluster.cs | Adds adaptive retries parameter passed to base cluster. |
| src/MongoDB.Driver/Core/Clusters/MultiServerCluster.cs | Adds adaptive retries parameter passed to base cluster; minor lock init cleanup. |
| src/MongoDB.Driver/Core/Clusters/LoadBalancedCluster.cs | Adds adaptive retries parameter and creates token bucket accordingly. |
| src/MongoDB.Driver/Core/Clusters/ICluster.cs | Exposes token bucket on internal cluster interface. |
| src/MongoDB.Driver/Core/Clusters/ClusterFactory.cs | Plumbs adaptive retries into cluster creation. |
| src/MongoDB.Driver/Core/Clusters/Cluster.cs | Creates token bucket based on adaptive retries and passes into servers. |
| src/MongoDB.Driver/Core/Bindings/WritableServerBinding.cs | Exposes cluster token bucket via binding. |
| src/MongoDB.Driver/Core/Bindings/SingleServerReadWriteBinding.cs | Exposes server token bucket via binding. |
| src/MongoDB.Driver/Core/Bindings/SingleServerReadBinding.cs | Exposes server token bucket via binding. |
| src/MongoDB.Driver/Core/Bindings/ReadWriteBindingHandle.cs | Exposes token bucket via binding handle. |
| src/MongoDB.Driver/Core/Bindings/ReadPreferenceBinding.cs | Exposes cluster token bucket via binding. |
| src/MongoDB.Driver/Core/Bindings/ReadBindingHandle.cs | Exposes token bucket via binding handle. |
| src/MongoDB.Driver/Core/Bindings/IBinding.cs | Adds TokenBucket to binding interface. |
| src/MongoDB.Driver/Core/Bindings/CoreTransaction.cs | Adds state-reset/flag hooks to support backpressure retry behavior. |
| src/MongoDB.Driver/Core/Bindings/ChannelSourceReadWriteBinding.cs | Removes binding implementation. |
| src/MongoDB.Driver/Core/Bindings/ChannelReadWriteBinding.cs | Exposes server token bucket via binding. |
| src/MongoDB.Driver/Core/Bindings/ChannelReadBinding.cs | Exposes server token bucket via binding. |
| src/MongoDB.Driver/ClusterRegistry.cs | Plumbs adaptive retries into cluster builder configuration. |
| src/MongoDB.Driver/ClusterKey.cs | Adds AdaptiveRetries to cluster key identity. |
| src/MongoDB.Driver/ChangeStreamHelper.cs | Sets CanBeRetried consistently for change stream operations. |
| specifications/uri-options/tests/client-backpressure-options.yml | Adds URI option tests for adaptiveRetries. |
| specifications/uri-options/tests/client-backpressure-options.json | Adds JSON form of URI option tests for adaptiveRetries. |
| specifications/transactions/tests/unified/backpressure-retryable-reads.yml | Adds unified transaction test for backpressure retry behavior on reads. |
| specifications/client-backpressure/tests/getMore-retried.yml | Adds spec tests ensuring getMore is retried under overload labels. |
| specifications/client-backpressure/tests/backpressure-retry-max-attempts.yml.template | Adds template for max-attempt retry behavior spec tests. |
| specifications/client-backpressure/tests/backpressure-connection-checkin.yml | Adds spec test ensuring connections are checked back in between retry attempts. |
| specifications/client-backpressure/tests/backpressure-connection-checkin.json | Adds JSON form of connection check-in retry spec test. |
Comments suppressed due to low confidence (5)
tests/MongoDB.Driver.Tests/Specifications/mongodb-handshake/prose-tests/MongoDbHandshakeProseTests.cs:131
- This new integration test is marked with TODOs and logs captured command documents at error level. Please remove the temporary logging/TODOs and make the assertions deterministic (e.g., assert exactly one hello/legacy hello was observed and that it contains
backpressure: true). Also, the spec link points to a personal fork; use the canonical mongodb/specifications link for the referenced test.
tests/MongoDB.Driver.Tests/Core/Operations/AsyncCursorTests.cs:406 - A previously enabled theory (
GetMore_should_use_same_session) is now commented out with a TODO, but the method remains in the file (and will no longer run). Since getMore retry/backpressure behavior is part of this change set, please fix the mocking issue and re-enable the test (or replace it with an equivalent, stable test) rather than leaving it disabled.
src/MongoDB.Driver/GridFS/GridFSSeekableDownloadStream.cs:74 _retryReadsis used to controlRetryRequested/CanBeRetried, but in this type it’s only backed by a property and is never initialized from client settings (and there’s a TODO noting this). As a result, retries for seekable GridFS downloads may always be disabled. Please wireRetryReadsfrom the bucket/client settings when creating this stream (or remove the field/property if not supported) and remove the TODO.
src/MongoDB.Driver/GridFS/GridFSBucket.cs:646- There’s a TODO noting that
RetryReadsis never passed/initialized when creating GridFS download streams. Given that downstream operations now useRetryRequested/CanBeRetried, please ensure the correct retry flags are propagated into both seekable and forward-only download streams (or explicitly document why they should remain non-retryable).
tests/MongoDB.Driver.Tests/Core/Connections/HelloHelperTests.cs:42 - These tests now assert that
backpressure: trueis always present in hello commands. Ifbackpressureis intended to be controlled by the newadaptiveRetriesoption, please extend the tests to cover both enabled/disabled cases (and avoid hard-coding unconditional presence).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
52
to
61
| var helloCommandName = helloOk || loadBalanced || serverApi != null ? "hello" : OppressiveLanguageConstants.LegacyHelloCommandName; | ||
| return new BsonDocument | ||
| { | ||
| { helloCommandName, 1 }, | ||
| { "helloOk", true }, | ||
| { "topologyVersion", () => topologyVersion.ToBsonDocument(), topologyVersion != null }, | ||
| { "maxAwaitTimeMS", () => (long)maxAwaitTime.Value.TotalMilliseconds, maxAwaitTime.HasValue }, | ||
| { "loadBalanced", true, loadBalanced } | ||
| { "loadBalanced", true, loadBalanced }, | ||
| { "backpressure", true } | ||
| }; |
There was a problem hiding this comment.
The hello command unconditionally includes backpressure: true. This makes the server treat the client as backpressure-aware even when adaptiveRetries is disabled (no token bucket), which can change server behavior and lead to unexpected overload errors/retries. Consider making backpressure conditional on the client/cluster configuration (e.g., only when adaptive retries/backpressure handling is enabled).
Comment on lines
75
to
80
| operationContext.ThrowIfTimedOutOrCanceled(); | ||
| var writeChannelSource = Binding.GetWriteChannelSource(operationContext, deprioritizedServers); | ||
| ReplaceChannelSource(writeChannelSource); | ||
| //TODO The implementation of the two overloads is different, need to understand if it's an issue and we can just call the second overload from the first one. | ||
| var writeChannelSource = _mayUseSecondaryCriteria == null ? | ||
| Binding.GetWriteChannelSource(operationContext, deprioritizedServers) | ||
| : Binding.GetWriteChannelSource(operationContext, deprioritizedServers, _mayUseSecondaryCriteria); ReplaceChannelSource(writeChannelSource); | ||
| return ChannelSource.ServerDescription; |
There was a problem hiding this comment.
SelectServer currently has a TODO and the conditional assignment/ReplaceChannelSource call are on the same line, which is easy to miss and makes the control flow harder to read/maintain. Please split the statement properly and either remove the TODO (by unifying the overload implementations) or add a tracking ticket/reference for the intended behavior.
Comment on lines
+306
to
+314
| public static class OperationRetryBackpressureConstants | ||
| { | ||
| //TODO Should these be here...? | ||
| public const int BasePowerBackoff = 2; | ||
| public const int InitialBackoff = 100; | ||
| public const int MaxBackoff = 10000; | ||
| public const int MaxRetries = 5; | ||
| public const double RetryTokenReturnRate = 0.1; | ||
| } |
There was a problem hiding this comment.
OperationRetryBackpressureConstants is a public nested type with an unresolved TODO. These constants look like driver-wide configuration knobs; consider moving them to a dedicated internal constants/configuration type (and/or making the nested type internal) and remove the TODO so the contract is clear.
Comment on lines
166
to
+184
| @@ -172,5 +179,8 @@ internal void UnpinAll() | |||
| _pinnedServer = null; | |||
| } | |||
| } | |||
|
|
|||
| //TODO Let's see if we keep this | |||
| internal bool HasCompletedCommand { get; set; } | |||
There was a problem hiding this comment.
ResetState and HasCompletedCommand are introduced with TODOs and mutate transaction internals directly. Since these are used to influence retry behavior, please either finalize and document the intended invariants (including thread-safety/locking expectations) or avoid adding partially-baked transaction state APIs to the core transaction type.
Comment on lines
279
to
292
| var cursorDocument = cursorElement.Value.AsBsonDocument; | ||
| return new AsyncCursor<BsonDocument>( | ||
| context.ChannelSource, | ||
| CollectionNamespace.FromFullName(cursorDocument["ns"].AsString), | ||
| null, | ||
| cursorDocument["firstBatch"].AsBsonArray.Cast<BsonDocument>().ToList(), | ||
| cursorDocument["id"].AsInt64, | ||
| 0, | ||
| 0, | ||
| BsonDocumentSerializer.Instance, | ||
| MessageEncoderSettings); | ||
| MessageEncoderSettings, | ||
| maxTime: null, | ||
| canBeRetried: RetryRequested); //TODO This is a little strange, but AsyncCursor do read operation, but this a write operation. Do we keep it like this...? It should be CanBeRetried anyways, once we add it to write operations. | ||
| } |
There was a problem hiding this comment.
AsyncCursor is constructed with canBeRetried: RetryRequested and a TODO notes the mismatch. Using RetryRequested here can enable/disable backpressure getMore retries inconsistently with the operation’s CanBeRetried policy. Please pass the correct retryability flag (and remove the TODO) so cursor retries are governed consistently.
Comment on lines
+1
to
4
| /* Copyright 2010-present MongoDB Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. |
There was a problem hiding this comment.
The file header now appears to contain multiple BOM/zero-width characters (�/double-BOM), which can cause noisy diffs and tooling/analyzer issues. Please normalize the file encoding so the header starts with a single standard UTF-8 BOM (or none, per repo convention).
Comment on lines
+24
to
+28
| private const double Capacity = 1000d; | ||
| private double _tokens = Capacity; | ||
|
|
||
| public double Tokens => _tokens; | ||
|
|
There was a problem hiding this comment.
Tokens returns _tokens without any memory barrier/atomic read. Since updates use Interlocked.CompareExchange, consider reading via Volatile.Read(ref _tokens) (or Interlocked.CompareExchange(ref _tokens, 0d, 0d)) to avoid torn/stale reads on runtimes where non-volatile 64-bit reads aren’t guaranteed atomic.
Comment on lines
+48
to
+56
| /// <summary> | ||
| /// This property only influences the retryability for retryable reads/writes and has no effect | ||
| /// on client backpressure errors. | ||
| /// </summary> | ||
| public bool RetryRequested => _retryRequested; | ||
| /// <summary> | ||
| /// Indicates whether the operation can be retried. If false, retries are disabled entirely. | ||
| /// </summary> | ||
| public bool CanBeRetried => _canBeRetried; |
There was a problem hiding this comment.
The XML doc for RetryRequested says it has no effect on client backpressure errors, but the retry executors gate all retries (including backpressure retries) on context.CanBeRetried/operation retryability. This comment is misleading—please update the documentation to reflect the actual behavior (or adjust the implementation if the intent is different).
Comment on lines
+52
to
+60
| /// <summary> | ||
| /// This property only influences the retryability for retryable reads/writes and has no effect | ||
| /// on client backpressure errors. | ||
| /// </summary> | ||
| public bool RetryRequested => _retryRequested; | ||
| /// <summary> | ||
| /// Indicates whether the operation can be retried. If false, retries are disabled entirely. | ||
| /// </summary> | ||
| public bool CanBeRetried => _canBeRetried; |
There was a problem hiding this comment.
The XML doc for RetryRequested says it has no effect on client backpressure errors, but backpressure retries are still gated by the operation/context retryability flags (e.g., CanBeRetried). Please update the documentation so it matches the actual retry semantics (or adjust the implementation if the intent is different).
Comment on lines
453
to
456
| var sentMessages = MessageHelper.TranslateMessagesToBsonDocuments(connection.GetSentMessages()); | ||
| var requestId = sentMessages[0]["requestId"].AsInt32; | ||
| sentMessages[0].Should().Be($"{{ opcode : \"opmsg\", requestId : {requestId}, responseTo : 0, exhaustAllowed : true, sections : [ {{ payloadType : 0, document : {{ hello : 1, helloOk: true, topologyVersion : {{ processId : ObjectId(\"000000000000000000000000\"), counter : NumberLong(0) }}, maxAwaitTimeMS : NumberLong(86400000), $db : \"admin\", apiVersion : \"1\" }} }} ] }}"); | ||
| sentMessages[0].Should().Be($"{{ opcode : \"opmsg\", requestId : {requestId}, responseTo : 0, exhaustAllowed : true, sections : [ {{ payloadType : 0, document : {{ hello : 1, helloOk: true, topologyVersion : {{ processId : ObjectId(\"000000000000000000000000\"), counter : NumberLong(0) }}, maxAwaitTimeMS : NumberLong(86400000), \"backpressure\" : true, $db : \"admin\", apiVersion : \"1\" }} }} ] }}"); | ||
| } |
There was a problem hiding this comment.
These assertions hard-code backpressure : true in the expected hello document. If backpressure advertisement becomes conditional on adaptiveRetries, this test should explicitly configure that setting (or assert conditionally) to avoid coupling the monitor tests to a global default.
sanych-sun
reviewed
Apr 9, 2026
| MessageEncoderSettings); | ||
| MessageEncoderSettings, | ||
| maxTime: null, | ||
| canBeRetried: RetryRequested); //TODO This is a little strange, but AsyncCursor do read operation, but this a write operation. Do we keep it like this...? It should be CanBeRetried anyways, once we add it to write operations. |
Member
There was a problem hiding this comment.
Accordingly to the spec (before Backpressure) whenever we run into an error while reading the result cursor - we have to restart the entire bulk write if it's retriable. With Backpressure we should retry read only if SystemOverloaded label is present.
https://github.com/mongodb/specifications/blob/master/source/crud/bulk-write.md#retrying-getmores
Member
There was a problem hiding this comment.
Please note there is additional check in GetEffectiveRetryRequested that validates if the bulk operation could be really retriable.
| NameOnly = options.NameOnly, | ||
| RetryRequested = _settings.RetryReads | ||
| RetryRequested = _settings.RetryReads, | ||
| CanBeRetried = _settings.RetryReads |
Member
There was a problem hiding this comment.
What the difference between RetryRequested and CanBeRetried?
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.
No description provided.