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

Choose pluck_script based on Redis version #58

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

russointroitoa
Copy link
Contributor

Since the updated LUA script coming from this PR uses commands available from Redis >= 6.2.0, with the following PR we select the old or new LUA script based on the Redis version

lib/sidekiq/grouping/redis.rb Outdated Show resolved Hide resolved
lib/sidekiq/grouping/redis.rb Outdated Show resolved Hide resolved
lib/sidekiq/grouping/redis.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
spec/modules/redis_spec.rb Outdated Show resolved Hide resolved
@ericproulx
Copy link

@russointroitoa any updates on that PR ?

Remove byebug and fix linting

Fix line too long

Add hashie dependency

Mock RedisClient instead of Hashie

Fix tests

Fix comparison

Fix indentation

Remove redundant curr directory path

Change constant name
@russointroitoa russointroitoa force-pushed the check_redis_version_for_lua_script branch from 69a3bb7 to b3497cd Compare September 16, 2024 05:02
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
@russointroitoa
Copy link
Contributor Author

russointroitoa commented Sep 16, 2024

@ericproulx Sorry for the long delay. The problem should be fixed now. My updates involve:

  • Split the Pluck Script into 2 versions
  • Update the gemspec by moving the development dependencies into the Gemfile
  • Fix installation path permissions for Github Actions

@ericproulx
Copy link

ericproulx commented Sep 17, 2024

@russointroitoa no worries thanks :)

Probably in another PR, you could load the script initially in redis instead of always passing the raw version.

I've seen this within the redlock gem that load the script if not present and use the SHA1 of that script.

Somehow, I don't know when the connection is established with Redis, but it could improve a little bit the network latency since you'll pass the SHA1 instead of the whole script every time.

I could give it a try if would like

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.

3 participants