CSHARP-3757: Redirect read/write retries to other mongos if possible #1304

adelinowona · 2024-04-10T23:13:23Z

Description

On a sharded cluster, retrying reads and writes will attempt to prioritize other mongos for selection if available.

What is changing?

Server selection now takes a collection of servers to deprioritize and will attempt to exclude those servers when the topology is sharded and multiple mongoses are present.
Adds unit tests for server selection and prose tests for retryable reads and writes.
Adds functionality to provide a collection of servers to deprioritize in relevant places.

adelinowona · 2024-04-10T23:22:45Z

tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs

+
+            var deprioritizedServers = new List<ServerDescription> { connected1 };
+
+            for (int i = 0; i < 15; i++)


Running multiple times to account for server selection randomness and guarantee we get the expected result for each run.

BorisDog

Looks good, few minor comments.

src/MongoDB.Driver.Core/Core/Bindings/IBinding.cs

src/MongoDB.Driver.Core/Core/Clusters/Cluster.cs

src/MongoDB.Driver.Core/Core/Operations/RetryableReadOperationExecutor.cs

tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs

src/MongoDB.Driver.Core/Core/Bindings/ChannelReadBinding.cs

src/MongoDB.Driver.Core/Core/Clusters/Cluster.cs

BorisDog · 2024-04-24T18:13:15Z

src/MongoDB.Driver.Core/Core/Clusters/ICluster.cs

        /// <returns>The selected server.</returns>
-        IServer SelectServer(IServerSelector selector, CancellationToken cancellationToken);
+        IServer SelectServer(IServerSelector selector, CancellationToken cancellationToken, IReadOnlyCollection<ServerDescription> deprioritizedServers = null);


Consider having augmenting IServerSelector to account for deprioritizedServers.

This reverts commit 1e88dc4.

This reverts commit 3e6d403.

This reverts commit 49995fd.

BorisDog

Some minor comments.

BorisDog · 2024-05-23T21:45:55Z

src/MongoDB.Driver.Core/Core/Bindings/WritableServerBinding.cs

+            var selector = deprioritizedServers != null
+                ? (IServerSelector)new CompositeServerSelector(new IServerSelector[] { new PriorityServerSelector(deprioritizedServers), writableServerSelector })
+                : writableServerSelector;
+


selector = deprioritizedServers != null ? new CompositeServerSelector(new[] { new PriorityServerSelector(deprioritizedServers), selector }) : selector;

new IServerSelector[] -> new[]

src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs

src/MongoDB.Driver.Core/Core/Clusters/ICluster.cs

src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs

src/MongoDB.Driver.Core/Core/Operations/RetryableReadOperationExecutor.cs

tests/MongoDB.Driver.Tests/Specifications/retryable-reads/RetryableReadsProseTests.cs

tests/MongoDB.Driver.Core.Tests/Core/Bindings/WritableServerBindingTests.cs

tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs

tests/MongoDB.Driver.Tests/Specifications/retryable-reads/RetryableReadsProseTests.cs

.../MongoDB.Driver.Tests/Specifications/retryable-writes/prose-tests/RetryWriteOnOtherMongos.cs

BorisDog · 2024-05-25T01:06:55Z

tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs

+
+        [Theory]
+        [ParameterAttributeData]
+        public async void SelectServer_should_return_deprioritized_servers_if_no_other_servers_exist_or_cluster_not_sharded(


Forgot to mention that async method should return Task, for better clarity.

BorisDog · 2024-05-25T01:07:37Z

src/MongoDB.Driver.Core/Core/Operations/RetryableWriteOperationExecutor.cs

@@ -53,7 +53,7 @@ public static TResult Execute<TResult>(IRetryableWriteOperation<TResult> operati

            try
            {
-                context.ReplaceChannelSource(context.Binding.GetWriteChannelSource(cancellationToken));
+                context.ReplaceChannelSource(context.Binding.GetWriteChannelSource(new [] { context.ChannelSource.ServerDescription }, cancellationToken));


Remove the extra space in new [] --> new[]

BorisDog · 2024-05-25T01:09:41Z

src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs

+    /// <summary>
+    /// Represents a server selector that selects servers based on a collection of servers to deprioritize.
+    /// </summary>
+    public sealed class PriorityServerSelector : IServerSelector


We probably should have few very basic tests for this selector. like EndPointServerSelectorTests

BorisDog · 2024-05-25T01:11:11Z

src/MongoDB.Driver.Core/Core/Bindings/WritableServerBinding.cs

+            var selector = deprioritizedServers != null
+                ? (IServerSelector)new CompositeServerSelector(new IServerSelector[] { new PriorityServerSelector(deprioritizedServers), writableServerSelector })
+                : writableServerSelector;
+


new IServerSelector[] -> new[]

BorisDog · 2024-05-25T01:12:04Z

src/MongoDB.Driver.Core/Core/Bindings/WritableServerBinding.cs

            return CreateServerChannelSource(server);
        }

        /// <inheritdoc/>
        public async Task<IChannelSourceHandle> GetWriteChannelSourceAsync(IMayUseSecondaryCriteria mayUseSecondary, CancellationToken cancellationToken)
+        {
+            return await GetWriteChannelSourceAsync(null, mayUseSecondary, cancellationToken).ConfigureAwait(false);


No need to await, can just return the task.
Additional await might create addition wrapping task object.

BorisDog · 2024-05-25T01:12:31Z

src/MongoDB.Driver.Core/Core/Bindings/WritableServerBinding.cs

+            var writableServerSelector = new WritableServerSelector(mayUseSecondary);
+
+            var selector = deprioritizedServers != null
+                ? (IServerSelector)new CompositeServerSelector(new IServerSelector[] { new PriorityServerSelector(deprioritizedServers), writableServerSelector })


Same comment applies here

BorisDog · 2024-05-25T01:14:48Z

src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs

+        /// <param name="deprioritizedServers">The collection of servers to deprioritize.</param>
+        public PriorityServerSelector(IReadOnlyCollection<ServerDescription> deprioritizedServers)
+        {
+            _deprioritizedServers = Ensure.IsNotNull(deprioritizedServers, nameof(deprioritizedServers));


Should this be IsNotNullOrEmpty maybe?

adelinowona added 4 commits April 10, 2024 18:56

CSHARP-3757: Redirect read/write retries to other mongos if possible

fd34bc0

Add selectServer unit tests

52539e9

Adds functionality for passing deprioritized servers in retries

cd47033

add new prose tests

464b498

adelinowona requested review from JamesKovacs and BorisDog April 10, 2024 23:13

adelinowona requested a review from a team as a code owner April 10, 2024 23:13

adelinowona commented Apr 10, 2024

View reviewed changes

adelinowona removed the request for review from a team April 10, 2024 23:24

adelinowona added 3 commits April 11, 2024 13:13

Fix failing evergreen tests

5efce80

change events capturing method for prose tests

d123f07

fix failing retryable prose tests on evergreen

dd82180

BorisDog requested changes Apr 16, 2024

View reviewed changes

adelinowona added 2 commits April 22, 2024 15:43

refactor deprioritization mechanism

bef0c37

Incorporate review feedback

49995fd

adelinowona requested a review from BorisDog April 23, 2024 18:38

adelinowona added 2 commits April 23, 2024 14:59

remove unnecessary white space

3e6d403

Update api docs and remove overloads

1e88dc4

BorisDog requested changes Apr 24, 2024

View reviewed changes

adelinowona added 5 commits April 29, 2024 18:47

Revert "Update api docs and remove overloads"

7394726

This reverts commit 1e88dc4.

Revert "remove unnecessary white space"

2f34978

This reverts commit 3e6d403.

Revert "Incorporate review feedback"

83eda80

This reverts commit 49995fd.

Use ServerSelector Pattern

883d0a7

Cleanup

a6a1569

adelinowona requested a review from BorisDog May 22, 2024 21:17

BorisDog requested changes May 23, 2024

View reviewed changes

BorisDog requested changes May 24, 2024

View reviewed changes

BorisDog reviewed May 24, 2024

View reviewed changes

tests/MongoDB.Driver.Tests/Specifications/retryable-reads/RetryableReadsProseTests.cs Outdated Show resolved Hide resolved

.../MongoDB.Driver.Tests/Specifications/retryable-writes/prose-tests/RetryWriteOnOtherMongos.cs Outdated Show resolved Hide resolved

resolve review comments

8de4da2

adelinowona requested a review from BorisDog May 24, 2024 23:37

BorisDog reviewed May 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-3757: Redirect read/write retries to other mongos if possible #1304

CSHARP-3757: Redirect read/write retries to other mongos if possible #1304

adelinowona commented Apr 10, 2024

adelinowona Apr 10, 2024

BorisDog left a comment

BorisDog Apr 24, 2024

BorisDog left a comment

BorisDog May 23, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024

BorisDog May 25, 2024


		var deprioritizedServers = new List<ServerDescription> { connected1 };

		for (int i = 0; i < 15; i++)

CSHARP-3757: Redirect read/write retries to other mongos if possible #1304

Are you sure you want to change the base?

CSHARP-3757: Redirect read/write retries to other mongos if possible #1304

Conversation

adelinowona commented Apr 10, 2024

Description

What is changing?

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment