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.

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

Do not send tracers when running second iteration of cluster nodes di… #2525

Merged
merged 2 commits into from
Sep 10, 2023

Conversation

Matiszak
Copy link
Contributor

…scovery (#2520)

Sending tracers is not necessary because we just connected to the nodes a few seconds ago. It causes problems because sent tracers do not have enough time to respond.

@Matiszak Matiszak force-pushed the develop branch 3 times, most recently from b1065a3 to 3d25f75 Compare August 16, 2023 14:46
@Matiszak
Copy link
Contributor Author

I'm currently testing the change below on your CI but it seems I somwhow broke 1/2 tests, I'm gonna check why:

// This awaits either the endpoint's initial connection, or a tracer if we're already connected
 // (which is the reconfigure case**, except second iteration which is only for newly discovered cluster members**).
var isFirstIteration = iter == 0;
available[i] = server.OnConnectedAsync(log, sendTracerIfConnected: isFirstIteration, autoConfigureIfConnected: reconfigureAll);

@Matiszak Matiszak force-pushed the develop branch 6 times, most recently from 10071f4 to 3f4f6d8 Compare August 16, 2023 16:37
…scovery (StackExchange#2520)

Sending tracers is not necessary because we just connected to the nodes a few seconds ago. It causes problems because sent tracers do not have enough time to respond.
@Matiszak
Copy link
Contributor Author

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Wanted to land RESP3 ahead to prevent conflicts here, but agree with this change. Thanks for the investigation and the improvement here!

@NickCraver NickCraver merged commit 5504ed9 into StackExchange:main Sep 10, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Race condition for being selectable when some nodes in the cluster are down
2 participants