CSHARP-3537: CSOT: retryable reads and writes #1717
Conversation
sanych-sun
requested review from
adelinowona and
BorisDog
and removed request for
papafe
| @@ -88,73 +80,82 @@ public RetryableWriteContext(IWriteBinding binding, bool retryRequested) | |||
| public IChannelSourceHandle ChannelSource => _channelSource; | |||
| public bool RetryRequested => _retryRequested; | |||
|
|
|||
| public void DisableRetriesIfAnyWriteRequestIsNotRetryable(IEnumerable<WriteRequest> requests) | |||
There was a problem hiding this comment.
As it turned out none of request.IsRetryable is using ConnectionDescription, so I've decided to remove this method and calculate _retryRequested upfront before the context creation.
| } | ||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
| internal async Task InitializeAsync(OperationContext operationContext, IReadOnlyCollection<ServerDescription> deprioritizedServers) |
There was a problem hiding this comment.
Initialize and InitializeAsync are internal now, because they have retry logic, that is also useful on operation retry, when we replacing the channel. Might need to be renamed though, because OperationExecutor could call this method several times.
There was a problem hiding this comment.
Yes, it needs a better naming.
| catch (Exception ex) when (ShouldThrowOriginalException(ex)) | ||
| { | ||
| throw originalException; | ||
| HashSet<ServerDescription> deprioritizedServers = null; |
There was a problem hiding this comment.
Accordingly to spec's pseudo code we should provide only one previous server on retry in the list of deprioritized servers, but as it was discussed in the feature channel - we ought to maintain the list of previously attempted servers, because the original code was created when there was only 1 retry. At least Java and Python maintain the list as well.
| while (true) | ||
| { | ||
| operationContext.ThrowIfTimedOutOrCanceled(); | ||
| var server = context.ChannelSource.ServerDescription; |
There was a problem hiding this comment.
Need to preserve the server description BEFORE the operation, because in case of exception server's description might be reset, but we need the original one to determinate if the operation could be retried.
| } | ||
| catch (Exception ex) | ||
| { | ||
| var innerException = ex is MongoAuthenticationException mongoAuthenticationException ? mongoAuthenticationException.InnerException : ex; |
There was a problem hiding this comment.
Why not to continue extracting the exception in ShouldRetryOperation? Just to reuse the code.
Then you also can keep using the When pattern, so more compact code.
src/MongoDB.Driver/Core/Operations/RetryableReadOperationExecutor.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
| internal async Task InitializeAsync(OperationContext operationContext, IReadOnlyCollection<ServerDescription> deprioritizedServers) |
There was a problem hiding this comment.
Yes, it needs a better naming.
tests/MongoDB.Driver.Tests/Core/Operations/RetryableWriteOperationExecutorTests.cs
Show resolved
Hide resolved
tests/MongoDB.Driver.Tests/Core/Operations/WriteCommandOperationTests.cs
Show resolved
Hide resolved
src/MongoDB.Driver/Core/Operations/RetryableReadOperationExecutor.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Driver/Core/Operations/RetryableReadOperationExecutor.cs
Outdated
Show resolved
Hide resolved
| { | ||
| internal static class OperationContextExtensions | ||
| { | ||
| public static bool IsOperationTimeoutConfigured(this OperationContext operationContext) |
There was a problem hiding this comment.
IsRootContextTimeoutConfigured?
No description provided.