-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fix deprecated method and testing #153
Fix deprecated method and testing #153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leedrum Thank you for the contribution! A couple small things to address, and then I think we can get this merged
@mcasper Thank you for the comments. I have fixed it. Could you take a look? |
lib/sidekiq/failures/sorted_entry.rb
Outdated
results = conn.zrangebyscore(Sidekiq::Failures::LIST_KEY, score, score) | ||
# after Redis v6.2.0, zrangebyscore is deprecated and zrange with BYSCORE is used | ||
results = if Gem::Version.new(conn.info["redis_version"].to_s) > Gem::Version.new('6.2.0') | ||
conn.zrange(Sidekiq::Failures::LIST_KEY, score, score, "BYSCORE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this needs some slight tweaks: https://rubydoc.info/gems/redis/Redis/Commands/SortedSets#zrange-instance_method
conn.zrange(Sidekiq::Failures::LIST_KEY, score, score, "BYSCORE") | |
conn.zrange(Sidekiq::Failures::LIST_KEY, score.to_i, score.to_i, by_score: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcasper I fixed the comment. tks |
The CI still doesn't pass. I will fix it |
…rrect params for zrange
@mcasper I have fixed the comments and the CI run successfully. |
changes:
minitest
has changed the name of the module fromMiniTest
toMinitest
zrangebyscore
of Redis is deprecated. It should bezrange
with 1 more params ("BYSCORE"
)Sidekiq
config has been changed (reverted, we will supportSidekiq 7.x
later)Sidekiq
URL
to matchRedis
default config at local.before fixing:
after fixing: