New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSHARP-3757: Redirect read/write retries to other mongos if possible #1304
base: master
Are you sure you want to change the base?
Conversation
|
||
var deprioritizedServers = new List<ServerDescription> { connected1 }; | ||
|
||
for (int i = 0; i < 15; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running multiple times to account for server selection randomness and guarantee we get the expected result for each run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, few minor comments.
/// <param name="deprioritizedServers">The deprioritized servers.</param> | ||
/// <param name="cancellationToken">The cancellation token.</param> | ||
/// <returns>A channel source.</returns> | ||
IChannelSourceHandle GetReadChannelSource(IReadOnlyCollection<ServerDescription> deprioritizedServers, CancellationToken cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface is pseudo public (going to be hidden in 3.0), and we are introducing a breaking change anyway.
So should we consider just adding a parameter to the existing method?
src/MongoDB.Driver.Core/Core/Operations/RetryableReadOperationExecutor.cs
Outdated
Show resolved
Hide resolved
|
||
subject.Initialize(); | ||
|
||
var endPoint1 = new DnsEndPoint("localhost", 27017); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: add simple test to validate deprioritizedServers == null
case
IServer result; | ||
if (async) | ||
{ | ||
result = subject.SelectServerAsync(selector, deprioritizedServers, CancellationToken.None).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the test return Task
, and just await here.
@@ -77,14 +76,14 @@ public void Dispose() | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public IChannelSourceHandle GetReadChannelSource(CancellationToken cancellationToken) | |||
public IChannelSourceHandle GetReadChannelSource(CancellationToken cancellationToken, IReadOnlyCollection<ServerDescription> deprioritizedServers = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a convention that cancellationToken
has to be a last parameter.
List<ServerDescription> filteredServers = new(); | ||
foreach (var description in _description.Servers) | ||
{ | ||
if (!deprioritizedServers.Contains(description, new ServerDescriptionComparer())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
micro-optimization: could use single instance of comparer.
Or maybe the following would be slightly simpler:
if (deprioritizedServers.All(d => d.EndPoint != description.Endpoint))
/// <returns>The selected server.</returns> | ||
IServer SelectServer(IServerSelector selector, CancellationToken cancellationToken); | ||
IServer SelectServer(IServerSelector selector, CancellationToken cancellationToken, IReadOnlyCollection<ServerDescription> deprioritizedServers = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider having augmenting IServerSelector
to account for deprioritizedServers
.
Description
On a sharded cluster, retrying reads and writes will attempt to prioritize other mongos for selection if available.
What is changing?