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.

Sign up for GitHub

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

Jump to bottom

fix(locksmith): replace deprecated rpoplpush w/lmove #781

Merged
merged 1 commit into from
May 23, 2023
Merged

fix(locksmith): replace deprecated rpoplpush w/lmove #781

merged 1 commit into from
May 23, 2023

Conversation

GabrielaGuedes
Copy link
Contributor

This PR replaces the occurrences of rpoplpush by lmove since the first one has been deprecated in Redis 6.2: https://redis.io/commands/rpoplpush/

@GabrielaGuedes GabrielaGuedes mentioned this pull request May 12, 2023
Closed
letiesperon
letiesperon reviewed May 12, 2023
View reviewed changes
lib/sidekiq_unique_jobs/locksmith.rb
@@ -312,7 +312,7 @@ def brpoplpush(conn, wait)
# @api private
#
def rpoplpush(conn)
conn.rpoplpush(key.queued, key.primed)
conn.lmove(key.queued, key.primed, "RIGHT", "LEFT")
Copy link

Choose a reason for hiding this comment

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

should we rename the method name too?

Copy link
Contributor Author

@GabrielaGuedes GabrielaGuedes May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the suggestion!! It got me thinking that if we changed the method name, we would also need to change the params it receives in order to be compliant with what lmove does. Do you think it is worth to do these changes?

Copy link

Choose a reason for hiding this comment

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

@GabrielaGuedes I'm not the best to say. Hopefully a maintainer can chime in.
I'm just grateful someone is solving this annoying warning

@letiesperon
Copy link

letiesperon commented May 22, 2023 •
edited
Loading

hey! could any reviewer take a look at this please? 😃💜
@GabrielaGuedes the linters fail

@GabrielaGuedes
Copy link
Contributor Author

hey! could any reviewer take a look at this please? 😃💜 @GabrielaGuedes the linters fail

the failure is not related to the changes done here, so I'll fix it in another PR!

@GabrielaGuedes
Copy link
Contributor Author

hey! could any reviewer take a look at this please? smileypurple_heart @GabrielaGuedes the linters fail

the failure is not related to the changes done here, so I'll fix it in another PR!

Here it is: #784

@GabrielaGuedes GabrielaGuedes force-pushed the replace-redis-deprecated-commands branch from d54d5ca to 07c9642 Compare May 23, 2023 14:07
@mhenrixon mhenrixon changed the title Replace rpoplpush (deprecated) by lmove fix(locksmith): replace deprecated rpoplpush w/lmove May 23, 2023
@mhenrixon mhenrixon merged commit b31e80b into mhenrixon:main May 23, 2023
@mhenrixon mhenrixon added the bug label May 23, 2023
@knightq
Copy link

knightq commented May 31, 2023

Thank you @GabrielaGuedes ! 🙏
@mhenrixon do you have any plan about when will this be released?
We are eager to see it released! :-)

@letiesperon
Copy link

@mhenrixon could we get a release please? :D

@mhenrixon mhenrixon mentioned this pull request Jun 19, 2023
Closed
semenyukdmitry pushed a commit to UniversalAvenue/sidekiq-unique-jobs that referenced this pull request Jul 7, 2023
@GabrielaGuedes @semenyukdmitry
fix(locksmith): replace deprecated rpoplpush w/lmove (mhenrixon#781)
9e4e293 
replace rpoplpush (deprecated) by lmove
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@letiesperon letiesperon letiesperon left review comments

Assignees

@GabrielaGuedes GabrielaGuedes

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GabrielaGuedes @letiesperon @knightq @mhenrixon