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 RedisLock (PP-1481) #2285

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Refactor RedisLock (PP-1481) #2285

wants to merge 3 commits into from

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Feb 14, 2025

Description

Small refactor to the RedisLock class.

  • Add a new parameter to the lock() function called raise_when_not_acquired that raises an exception if the lock isn't acquired.
  • Make the TaskLock class only optionally take a redis_client parameter.

Motivation and Context

This is a small refactor to attempt to DRY some uses of the RedisLock classes.

Almost all the uses of RedisLock have the same pattern where they try to acquire the lock, then they raise (or exit) if the lock wasn't acquired. Since we are doing this everywhere anyway, this should be the default behavior. I added raise_when_not_acquired to do this, and refactored existing code to use it.

Note: This changes the behavior of some tasks slightly. They used to silently exit if they couldn't acquire a lock, now they will exit with an error. Since we don't expect tasks to fail to get the lock, I think this is the correct thing to do, but it is a slight change.

The TaskLock class used to take a required redis_client parameter. Since it also takes a Task parameter, which has access to the redis client, we make it optional, and fall back to the redis client on the task.

How Has This Been Tested?

  • Unit tests

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen requested a review from a team February 14, 2025 18:53
@jonathangreen jonathangreen changed the title Refactor RedisLock Refactor RedisLock (PP-1481) Feb 14, 2025
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.12%. Comparing base (e07e2cd) to head (d29308f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2285   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files         363      363           
  Lines       41330    41333    +3     
  Branches     8846     8846           
=======================================
+ Hits        37660    37663    +3     
  Misses       2408     2408           
  Partials     1262     1262           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good. 🎸

Just one minor typo in a comment.

assert not acquired

# If the raise_when_not_acquired parameter is set, the context manager raises
# an exception is the lock is not acquired.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo here: "...[if] the lock..."

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