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

Refactor key-slot conversion logic #301

Merged

Conversation

KJTsanaktsidis
Copy link
Contributor

Previously, extract_first_key would perform hashtag extraction on the key it pulled from command; that meant that the "key" it was returning might not actually be the key in the command.

This commit refactors things so that

  • extract_first_key actually extracts the first key
  • KeySlotConverter.convert does hashtag extraction
  • Router's find_node_key and find_primary_node_key can now be implemented in terms of a function "find_node_by_key", because they actually get the key from the command.

This is helpful for the cluster transaction support work, because it means we can re-use the keyslot/hashtag logic for transaction node assignment.

@supercaracal
Copy link
Member

supercaracal commented Dec 12, 2023

RedisClient::Cluster::TestCommand#test_extract_hash_tag:
NoMethodError: undefined method `extract_hash_tag'

It seems that the above test case is failure. Also, we can ignore Rubocop errors at this time because they aren't related to this refactoring.

Previously, extract_first_key would perform hashtag extraction on the
key it pulled from command; that meant that the "key" it was returning
might not actually be the key in the command.

This commit refactors things so that

* extract_first_key actually extracts the first key
* KeySlotConverter.convert does hashtag extraction
* Router's find_node_key and find_primary_node_key can now be
  implemented in terms of a function "find_node_by_key", because they
  _actually_ get the key from the command.
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/keyslot_refactor branch from c7464e7 to 31968f3 Compare December 12, 2023 08:47
@KJTsanaktsidis
Copy link
Contributor Author

Apologies, I made a mistake while resolving conflicts from my cherry-pick. The test should be fixed now.

@supercaracal supercaracal merged commit 8a3d03e into redis-rb:master Dec 12, 2023
25 checks passed
@supercaracal
Copy link
Member

Thank you!

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.

2 participants