Repository navigation
Fix(redis): Makes redis leader-election lease duration configurable - #157
MongareAlex wants to merge 10 commits into
Conversation
The Redis provider previously used a hardcoded 120-second TTL for the leadership key. A fixed 120s left no room to tune for this. Add RedisLeaderElectionOptions.LeaseDuration (default: 2 minutes) and thread it through acquire and renew paths in place of the literal.
The Redis provider previously used a hardcoded 120-second TTL for the leadership key. A fixed 120s left no room to tune for this. Add RedisLeaderElectionOptions.LeaseDuration (default: 2 minutes) and thread it through acquire and renew paths in place of the literal. Fixes steingran#156
…m/MongareAlex/MultiLock into fix/configurable-redis_leader_ttl
steingran
left a comment
steingran
left a comment
There was a problem hiding this comment.
Thanks for picking up #156! Making the renewal TTL configurable is a good change, and keeping the 2-minute default means existing users see no difference.
Main issue to fix before merge: LeaseDuration is only used when the leader renews the key. When the key is first created in TryAcquireLeadershipAsync, the TTL still comes from the lockTimeout argument (LeaderElectionOptions.LockTimeout, 30 s by default). The PR description says both paths are covered, but only the renew path changed.
Because of this, the new integration test TryAcquireLeadership_WithShortLeaseDuration_ShouldExpireWithoutHeartbeat will fail when it runs against Redis: the key is created with a 5-minute TTL, so it still exists after the 3-second wait.
Summary of the inline comments:
- Use
LeaseDurationon acquire too (or, if renewal-only is the intent, update the PR description and the test). - Make the integration test match what the code does.
- Add a test for the renew path. It is the only production line that changed, and no test covers it yet.
- Add unit tests for the new
LeaseDuration <= 0check. - The doc says
LeaseDurationmust be larger thanHeartbeatInterval, but nothing checks this. - A few small formatting nits (one-click suggestions).
Happy to answer questions on any of these.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 72.53% 72.75% +0.21%
==========================================
Files 65 65
Lines 4974 4977 +3
Branches 607 608 +1
==========================================
+ Hits 3608 3621 +13
+ Misses 1155 1145 -10
Partials 211 211
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
LeaseDuration now controls the TTL on both initial acquire and subsequent heartbeat renewals, so one setting governs the full lease window.
Behavior change: before this PR, the initial lease TTL was lockTimeout (30s default from LeaderElectionOptions). It is now LeaseDuration (2 min default).
Fixes #156