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.
| { | ||
| internal static class OperationContextExtensions | ||
| { | ||
| public static bool IsOperationTimeoutConfigured(this OperationContext operationContext) |
There was a problem hiding this comment.
Should this be called IsRootOperationTimeoutConfigured ?
nit: Consider
IsOperationTimeoutConfigured(this OperationContext operationContext) =>
operationContext.GetRootOperationContext().Timeout != Timeout.InfiniteTimeSpan;
Also consider just having IsRootTimeoutConfigured/RootContext property for simplicity.
There was a problem hiding this comment.
Changed OperationContext to have RootContext. Decided not to change the method name though, as we have a single operation, where the timeout might be configured.
| } | ||
| 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.
| return await ExecuteAsync(operationContext, operation, context).ConfigureAwait(false); | ||
| try | ||
| { | ||
| context.Initialize(operationContext, deprioritizedServers); |
There was a problem hiding this comment.
Probably need a better naming. Initialize can run only once.
| } | ||
| } | ||
| } | ||
|
|
||
| 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.
| @@ -13,17 +13,14 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| using System.Linq; | |||
| using System; | |||
| using System.Collections.Generic; | |||
| @@ -13,6 +13,7 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| using System.Collections.Generic; | |||
No description provided.