Clusters should optionally require full slot coverage #1845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…on, Changed RedisClient's 'require_full_coverage' default value to false
Codecov Report
@@ Coverage Diff @@
## master #1845 +/- ##
==========================================
- Coverage 93.64% 93.60% -0.05%
==========================================
Files 76 76
Lines 16207 16199 -8
==========================================
- Hits 15177 15163 -14
- Misses 1030 1036 +6
Continue to review full report at Codecov.
|
barshaul
commented
Jan 2, 2022
| @@ -1450,29 +1419,9 @@ def initialize(self): | |||
| # isn't a full coverage | |||
| raise RedisClusterException( | |||
| f"All slots are not covered after query all startup_nodes. " | |||
| f"{len(self.slots_cache)} of {REDIS_CLUSTER_HASH_SLOTS} " | |||
| f"{len(tmp_slots)} of {REDIS_CLUSTER_HASH_SLOTS} " | |||
There was a problem hiding this comment.
Makes sense - limit based on available as opposed to the entire cluster. This should recover more cleanly.
chayim
approved these changes
Jan 4, 2022
| @@ -1450,29 +1419,9 @@ def initialize(self): | |||
| # isn't a full coverage | |||
| raise RedisClusterException( | |||
| f"All slots are not covered after query all startup_nodes. " | |||
| f"{len(self.slots_cache)} of {REDIS_CLUSTER_HASH_SLOTS} " | |||
| f"{len(tmp_slots)} of {REDIS_CLUSTER_HASH_SLOTS} " | |||
There was a problem hiding this comment.
Makes sense - limit based on available as opposed to the entire cluster. This should recover more cleanly.
chayim
changed the title
dvora-h
pushed a commit
to dvora-h/redis-py
that referenced
this pull request
Jan 11, 2022
dvora-h
pushed a commit
to dvora-h/redis-py
that referenced
this pull request
Jan 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…on, Changed RedisClient's 'require_full_coverage' default value to false
Pull Request check-list
Please make sure to review and check all of these items:
$ toxpass with this change (including linting)?NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
In order to avoid the client being a limiting factor for communicating with the server, we should not require full slots coverage by default in the client side.
require_full_coverage parameter:
When set to False (default value): the client will not require a
full coverage of the slots. However, if not all slots are covered,
and at least one node has 'cluster-require-full-coverage' set to
'yes,' the server will throw a ClusterDownError for some key-based
commands. See -
https://redis.io/topics/cluster-tutorial#redis-cluster-configuration-parameters
When set to True: all slots must be covered to construct the
cluster client. If not all slots are covered, RedisClusterException
will be thrown.
The 'skip_full_coverage_check' parameter has been removed from RedisClient, and the client no longer checks for 'cluster-require-full-coverage' configuration on the cluster nodes (note - not backward compatible). This check doesn't need to be run since it's an overhead for the client and is already handled by the server's ClusterDown error. Additionally, some managed Redis clusters, such as ElastiCache, don't support the 'config' command, so the client will not work with the default configurations if we keep the node's configuration check. Other clients (e.g. Lettuce, Jedis) doesn't run this check as well.