Skip to content

Ensure context isn't exhausted via concurrent query as opposed to sentinel query #3334

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

Merged
merged 12 commits into from
Apr 16, 2025

Conversation

gnpaone
Copy link
Contributor

@gnpaone gnpaone commented Apr 5, 2025

Switch from sequential sentinel query to concurrent query to avoid context exhaustion and connect to new master redis after redis failover

@gnpaone
Copy link
Contributor Author

gnpaone commented Apr 5, 2025

Hey @ndyakov tried to fix your review points in #3174

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Please add a test , it may be a setup of three sentinels, two of which are unreachable. Define the context in such a way, to get context.DeadlineExceeded error on GetMasterAddrByName. Such setup should fail prior to your PR and succeeded with the introduced change, correct?

@kwenzh
Copy link

kwenzh commented Apr 10, 2025

Please add a test , it may be a setup of three sentinels, two of which are unreachable. Define the context in such a way, to get context.DeadlineExceeded error on GetMasterAddrByName. Such setup should fail prior to your PR and succeeded with the introduced change, correct?

The current problem is that the faulty sentinel node is in the front, which will cause context exhaustion. Even if the last sentinel can correctly provide the master address, don't we consider this situation?

@kwenzh
Copy link

kwenzh commented Apr 11, 2025

I think that once any sentinel returns the master address correctly, it should be considered finished and other coroutines should be notified of the end. Even if the context timeout occurs afterwards, we should return the master address that has been queried, right?

@kwenzh
Copy link

kwenzh commented Apr 11, 2025

I think that once any sentinel returns the master address correctly, it should be considered finished and other coroutines should be notified of the end. Even if the context timeout occurs afterwards, we should return the master address that has been queried, right?

with issue: #3172

@gnpaone
Copy link
Contributor Author

gnpaone commented Apr 11, 2025

I think that once any sentinel returns the master address correctly, it should be considered finished and other coroutines should be notified of the end. Even if the context timeout occurs afterwards, we should return the master address that has been queried, right?

with issue: #3172

Maybe should I use something like

ctx, cancel := context.WithCancel(ctx)
defer cancel()

...

cancel()

to stop goroutines early, once success is achieved ie sentinel returns the master address correctly?

@kwenzh
Copy link

kwenzh commented Apr 11, 2025

once success is achieved ie sentinel returns the master address correctly?

yeah,once success returns the master address correctly,That's exactly what I meant!

@gnpaone
Copy link
Contributor Author

gnpaone commented Apr 13, 2025

Done

@gnpaone gnpaone requested review from kwenzh and ndyakov April 13, 2025 12:03
@ndyakov
Copy link
Member

ndyakov commented Apr 15, 2025

@gnpaone can you please add the issue in the description of this PR. I will review the latest changes later today and get back to you if there are any comments.

@gnpaone
Copy link
Contributor Author

gnpaone commented Apr 16, 2025

Updated the description @ndyakov

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.

3 participants