-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Optimizing cluster initialization changing the checks for cluster-enabled flag #3158
Conversation
…-checked change if the cluster-mode is enabled by trying run CLUSTER SLOT inst…
fixing cluster mode is not enabled on this node tests
@dvora-h Almost all GitHub Actions have been cancelled. Is there something missing in MR to execute the actions? Another question, I noticed this same behavior in async, but I haven't made the change yet because I thought it could result in strange behavior. I was unsure whether async should have the same behavior given that it is asynchronous. |
@willianmrs Thanks for this PR! I like this approach. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3158 +/- ##
==========================================
- Coverage 91.84% 91.81% -0.04%
==========================================
Files 128 128
Lines 33232 33328 +96
==========================================
+ Hits 30523 30600 +77
- Misses 2709 2728 +19 ☔ View full report in Codecov by Sentry. |
@willianmrs also - notice the linters failure |
@dvora-h great. About async, I'm working on the async cluster changes. I guess in the first look at the Async connection I understand wrong. But it does the same as the sync cluster, so you are right, the same should work for it. And thanks about the lint, I will submit the fix with the async changes. |
… connection, now only the CLUSTER_SLOT, so the total commands is minus 1
optimizing async cluster creation using CLUSTER SLOT command instead …
…bled flag (redis#3158) * change if the cluster-mode is enabled by trying run CLUSTER SLOT insted of INFO * fix typo * fixing cluster mode is not enabled on this node tests * remove changes on asyncio * rename mock flag to be more consistent * optimizing async cluster creation using CLUSTER SLOT command instead of INFO command * fixing test. Before INFO and CLUSTER_SLOT was used for performing the connection, now only the CLUSTER_SLOT, so the total commands is minus 1 * remove dot at the end of string * remove unecessary print from test * fix lint problems --------- Co-authored-by: Willian Moreira <[email protected]>
…bled flag (#3158) * change if the cluster-mode is enabled by trying run CLUSTER SLOT insted of INFO * fix typo * fixing cluster mode is not enabled on this node tests * remove changes on asyncio * rename mock flag to be more consistent * optimizing async cluster creation using CLUSTER SLOT command instead of INFO command * fixing test. Before INFO and CLUSTER_SLOT was used for performing the connection, now only the CLUSTER_SLOT, so the total commands is minus 1 * remove dot at the end of string * remove unecessary print from test * fix lint problems --------- Co-authored-by: Willian Moreira <[email protected]>
…bled flag (#3158) * change if the cluster-mode is enabled by trying run CLUSTER SLOT insted of INFO * fix typo * fixing cluster mode is not enabled on this node tests * remove changes on asyncio * rename mock flag to be more consistent * optimizing async cluster creation using CLUSTER SLOT command instead of INFO command * fixing test. Before INFO and CLUSTER_SLOT was used for performing the connection, now only the CLUSTER_SLOT, so the total commands is minus 1 * remove dot at the end of string * remove unecessary print from test * fix lint problems --------- Co-authored-by: Willian Moreira <[email protected]>
…bled flag (#3158) * change if the cluster-mode is enabled by trying run CLUSTER SLOT insted of INFO * fix typo * fixing cluster mode is not enabled on this node tests * remove changes on asyncio * rename mock flag to be more consistent * optimizing async cluster creation using CLUSTER SLOT command instead of INFO command * fixing test. Before INFO and CLUSTER_SLOT was used for performing the connection, now only the CLUSTER_SLOT, so the total commands is minus 1 * remove dot at the end of string * remove unecessary print from test * fix lint problems --------- Co-authored-by: Willian Moreira <[email protected]>
…bled flag (#3158) * change if the cluster-mode is enabled by trying run CLUSTER SLOT insted of INFO * fix typo * fixing cluster mode is not enabled on this node tests * remove changes on asyncio * rename mock flag to be more consistent * optimizing async cluster creation using CLUSTER SLOT command instead of INFO command * fixing test. Before INFO and CLUSTER_SLOT was used for performing the connection, now only the CLUSTER_SLOT, so the total commands is minus 1 * remove dot at the end of string * remove unecessary print from test * fix lint problems --------- Co-authored-by: Willian Moreira <[email protected]>
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
In this PR, I propose an optimized approach to RedisCluster initialization, focusing on improving the speed and efficiency for clusters. The key change involves replacing the initial INFO command check for cluster mode enablement with a direct attempt to execute "CLUSTER SLOTS". This method assumes that successful execution of "CLUSTER SLOTS" implies cluster mode is active, thus streamlining the initialization process. This optimization aims to reduce startup time without compromising the integrity of cluster checks, making it particularly beneficial for environments where rapid deployment and high performance are critical.
To compare an INFO command takes an average of 2 to 3 seconds to complete and REDIS SLOTS takes only a few milliseconds. This enables the timeout being used in the constructor, not being necessary to add a bigger timeout just to initialize the cluster.
Linked issue:
#3148