CSHARP-5608: CSOT: Command Execution #1716
Conversation
sanych-sun
requested review from
papafe,
rstam and
adelinowona
and removed request for
a team and
papafe
There was a problem hiding this comment.
Pull Request Overview
This PR introduces explicit handling of server round-trip time ("RTT") by expanding SelectServer to return both a server and its RTT, and thread this RTT through various APIs and wire protocols to support operation timeouts and fail points.
- Update
SelectServerandSelectServerAsyncto return(IServer server, TimeSpan roundTripTime). - Add
serverRoundTripTimeparameters toFailPoint.Configure, binding constructors, andCommandWireProtocol. - Inject RTT into command message sections for dynamic timeout calculation and update numerous tests to use the new overloads.
Reviewed Changes
Copilot reviewed 103 out of 103 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTargetedFailPointOperation.cs | Destructure (server, roundTripTime) and pass RTT into FailPoint.Configure |
| src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs | Thread serverRoundTripTime into command creation and timeout logic |
| src/MongoDB.Driver/OperationContextExtensions.cs | Add HasOperationTimeout and root context lookup |
| src/MongoDB.Driver/Core/WireProtocol/CommandWireProtocol.cs | Update IWireProtocol methods to accept OperationContext and RTT |
| tests/MongoDB.Driver.Tests/Core/Bindings/SingleServerReadWriteBindingTests.cs | Introduce RTT parameter in test constructors and add validation |
Comments suppressed due to low confidence (3)
src/MongoDB.Driver/Core/WireProtocol/CommandWireProtocol.cs:88
- [nitpick] The field
_serverRoundTripTimeis declared without XML doc. Add a brief comment explaining that this RTT is used for dynamic timeout calculations in command messages.
messageEncoderSettings,
src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs:381
- OperationContext does not define a
RemainingTimeoutproperty. You should compute the remaining time from the original timeout and elapsed time, or add a helper onOperationContextto expose the remaining timeout.
var serverTimeout = operationContext.RemainingTimeout - _serverRoundTripTime;
src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs:630
RemainingTimeoutis not a member ofOperationContext. Consider replacing this check with your new helper (e.g.,HasOperationTimeout) or implement a properRemainingTimeoutgetter.
if (operationContext.RemainingTimeout == Timeout.InfiniteTimeSpan)
src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Driver/Core/WireProtocol/CommandUsingCommandMessageWireProtocol.cs
Show resolved
Hide resolved
| EndServerSelection(expirableClusterDescription.ClusterDescription, selector, result.ServerDescription, stopwatch); | ||
| return result.Server; | ||
| EndServerSelection(expirableClusterDescription.ClusterDescription, selector, description, stopwatch); | ||
| return (server, description.AverageRoundTripTime); |
There was a problem hiding this comment.
Accordingly to CSOT spec we should use MinRoundTripTime here, but CSharp driver do not track it yet, we have only AverageRoundTripTime. We decided to go with the AverageRoundTripTime and replace it on the later stages of CSOT implementation. More over there is a chance that RTT will be removed from maxTimeMs calculation at all.
Find more details in the https://jira.mongodb.org/browse/CSHARP-5627
| @@ -61,10 +64,45 @@ public TimeSpan RemainingTimeout | |||
| } | |||
| } | |||
|
|
|||
| [Obsolete("Do not use this property, unless it's needed to avoid breaking changes in public API")] | |||
| public CancellationToken CombinedCancellationToken | |||
There was a problem hiding this comment.
We have a number of use-cases when we cannot update the interfaces, because they are public (I've run into at least 2 cases like that: IStreamFactory and ISaslStep), so we have to squeeze both: cancellation token itself and timeout into linked cancellation token. Having such CombinedCancellationToken on operation context allow us to reuse instantiated CancellationTokenSources. Also I've marked the property as obsolete, so we easily can find all usages and replace with some interface changes in next major release.
| @@ -94,7 +115,7 @@ public void WaitTask(Task task) | |||
| } | |||
|
|
|||
| var timeout = RemainingTimeout; | |||
| if (timeout != System.Threading.Timeout.InfiniteTimeSpan && timeout < TimeSpan.Zero) | |||
There was a problem hiding this comment.
RemainingTimeout returns TimeSpan.Zero if timed out now, so the check could be simplified.
| { | ||
| var remainingTimeout = RemainingTimeout; | ||
| if (remainingTimeout == System.Threading.Timeout.InfiniteTimeSpan) |
There was a problem hiding this comment.
RemainingTimeout returns TimeSpan.Zero if timed out now, so the check could be simplified.
No description provided.