Skip to content

Conversation

@tarunbhardwaj
Copy link

No description provided.

Copy link
Member

@jkemp101 jkemp101 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I am wondering if we should leave setting the hash tag via {} to the caller and not assume it is the whole throttle name without the prefix. I assume the only change that is needed to make it be compatible with a cluster is moving ARGV[1] to KEYS[1] so it can route to the right node?

The Lua script docstring needs updating for the change to ARGV and KEYS.

@tarunbhardwaj
Copy link
Author

We can leave hash tag decision to caller, but we will have to make sure that hash is always provided to make this compatible with redis-cluster, as Lua script will touch other keys which might not be available in same node and that will raise error. If we ensure { } is passed we should be good. Any suggestion on handling that?

@jkemp101
Copy link
Member

After thinking about this a little more maybe leaving it to the caller isn't the best idea. At a minimum it makes the caller need to know about how hash tags are used in Redis clusters.

  • Explicitly pass in both keys to the Lua script so it is clear what is being used (less magic in the Lua script the better). So move this line to the Python side.
  • Include the prefix in the hash tag: '{{throttle:{}}}'. At some point we could allow setting the prefix and I would assume you would want them included in the hash decision.
  • Add a note to the readme that explains the use of the hash tag in the key name for use with Redis clusters.
  • We could add redis_clustering=False as an argument to throttle_configure() that can be used to enable the use of hash tags. That might make key names cleaner for non-cluster users.

Thoughts?

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