Skip to content

add readOnly on failover opts #3281

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 24 commits into from
Mar 6, 2025
Merged

add readOnly on failover opts #3281

merged 24 commits into from
Mar 6, 2025

Conversation

smilad
Copy link
Contributor

@smilad smilad commented Feb 18, 2025

fixing issues :
#3277

adding ReadOnly for scenarios where you need to connect to read-only replicas is not supported in UniversalClient

by adding this I can connect to readOnly and Master separately

add failover that if is True it connects to slaves
@ndyakov
Copy link
Member

ndyakov commented Feb 19, 2025

Hello @smilad, thank you for the contribution. Would you please add a test for the change that you are introducing in this PR?

@smilad
Copy link
Contributor Author

smilad commented Feb 19, 2025

@ndyakov :
Done

@ndyakov
Copy link
Member

ndyakov commented Feb 20, 2025

@smilad The test you added is skipped and won't be executed. Can we have a test that will be executed and validate the new option?

@smilad
Copy link
Contributor Author

smilad commented Feb 22, 2025

@ndyakov : it is because of your sentinel options
and pipelines have problems
the first test case also has this issue
but if run test on local it is ok

@ndyakov
Copy link
Member

ndyakov commented Feb 24, 2025

@smilad lets work together on making sure we can execute the desired test in CI. Looking at your change, having a test that tries to write to a read only client should result in an error. We can test for that? Also we can make sure we are connected to a replica. Correct?

@ndyakov
Copy link
Member

ndyakov commented Feb 26, 2025

@smilad i can see a binary that needs to be removed from the changes. Also, let's enable the test and work on making it pass.

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.

enable test

enable previously skipped test.
@ndyakov
Copy link
Member

ndyakov commented Feb 26, 2025

@smilad I see your point about the CI, there are some changes coming in the following weeks that should enable you to write tests agains sentinels. Are you OK with waiting #3274 to be merged before we can work on merging this one?

@smilad smilad requested a review from ndyakov February 27, 2025 18:59
@smilad
Copy link
Contributor Author

smilad commented Feb 27, 2025

@ndyakov
it didn't fixed
You must add slaves too
in code if replicas was empty it connect to masterAddr
you just add some master sentinel

don't run sentinel test in RE
@smilad
Copy link
Contributor Author

smilad commented Mar 1, 2025

@ndyakov
Is it ok to merge?

@ndyakov
Copy link
Member

ndyakov commented Mar 4, 2025

@smilad should be, can you please fix the formatting(indentations) on the test?

@ndyakov ndyakov self-requested a review March 6, 2025 14:52
@ndyakov ndyakov merged commit 8fadbef into redis:master Mar 6, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants