Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 24 commits into from

Conversation

adelinowona
Copy link
Contributor

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 adelinowona requested a review from a team as a code owner April 10, 2024 23:13
adelinowona
adelinowona commented Apr 10, 2024
View reviewed changes
tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs

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

for (int i = 0; i < 15; i++)
Copy link
Contributor Author

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.

@adelinowona adelinowona removed the request for review from a team April 10, 2024 23:24
BorisDog
BorisDog requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@BorisDog BorisDog left a 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.

src/MongoDB.Driver.Core/Core/Bindings/IBinding.cs Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/Cluster.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Operations/RetryableReadOperationExecutor.cs Outdated Show resolved Hide resolved
tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs Outdated Show resolved Hide resolved
tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs Outdated Show resolved Hide resolved
tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs Outdated Show resolved Hide resolved
tests/MongoDB.Driver.Core.Tests/Core/Clusters/ClusterTests.cs Outdated Show resolved Hide resolved
@adelinowona adelinowona requested a review from BorisDog May 22, 2024 21:17
BorisDog
BorisDog requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments.

src/MongoDB.Driver.Core/Core/Bindings/WritableServerBinding.cs Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/ICluster.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Clusters/ServerSelectors/PriorityServerSelector.cs Outdated Show resolved Hide resolved
src/MongoDB.Driver.Core/Core/Operations/RetryableReadOperationExecutor.cs Outdated Show resolved Hide resolved
@adelinowona adelinowona requested a review from BorisDog May 24, 2024 23:37
@adelinowona adelinowona requested a review from BorisDog May 28, 2024 20:27
BorisDog
BorisDog approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.IsNotNullOrEmpty(deprioritizedServers, nameof(deprioritizedServers)) as IReadOnlyCollection<ServerDescription>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we had appropriate overload, to skip the as .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't our list of deprioritized servers be empty? Wouldn't this be the case on the first call and wouldn't it only contain servers on the subsequent retry?

Right now 26 out of 28 patch variants are failing because deprioritizedServers is empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PriorityServerSelector is created conditionally (deprioritizedServers != null), so extending this condition to involve PriorityServerSelector only when deprioritizedServers is not empty seems natural.

JamesKovacs
JamesKovacs requested changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the logic around PriorityServerSelector isn't quite right as we have a lot of failing variants complaining about deprioritizedServers being empty. Please investigate and resolve. Thanks.

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

/// <inheritdoc/>
public override string ToString() => "PriorityServerSelector";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"PriorityServerSelector" => nameof(PriorityServerSelector)

We should also output _deprioritizedServers for debug purposes.

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.IsNotNullOrEmpty(deprioritizedServers, nameof(deprioritizedServers)) as IReadOnlyCollection<ServerDescription>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't our list of deprioritized servers be empty? Wouldn't this be the case on the first call and wouldn't it only contain servers on the subsequent retry?

Right now 26 out of 28 patch variants are failing because deprioritizedServers is empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BorisDog BorisDog BorisDog approved these changes

@JamesKovacs JamesKovacs Awaiting requested review from JamesKovacs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@adelinowona @JamesKovacs @BorisDog