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 Lease release/acquire operation logic #1289
Fix Lease release/acquire operation logic #1289
Conversation
* Lease release op when lease not acquired is a no-op * Lease acquire when lease was not acquired returns idempotent operation task instead of timing out
return true; | ||
case LeaseActor.InvalidReleaseRequest: | ||
_log.Info("Tried to release a lease that is not acquired"); | ||
return 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.
Logically, since we didn't have the lease, we should return true.
This will cause the plugins to report that the lease have been successfully released, which can be confusing.
This is why the log should be emitted in info level so that we can track that the lease was "successfully released" because we didn't have it in the first place.
@@ -109,25 +120,59 @@ public override async Task<bool> Release() | |||
public override Task<bool> Acquire() | |||
=> Acquire(null); | |||
|
|||
public override async Task<bool> Acquire(Action<Exception?>? leaseLostCallback) | |||
public override Task<bool> Acquire(Action<Exception?>? leaseLostCallback) |
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.
To make this idempotent, we could not use async...await pattern
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.
LGTM
{ | ||
if(_log.IsDebugEnabled) | ||
_log.Debug("Lease is already being acquired"); | ||
return _acquireTask; |
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.
LGTM
case LeaseActor.LeaseReleased: | ||
return true; | ||
case LeaseActor.InvalidReleaseRequest: | ||
_log.Info("Tried to release a lease that is not acquired"); |
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.
I'd mark this as DEBUG
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.
it needs to be INFO
because if its not, we could not debug if a lease was "released" because it was actually released or because we didn't have it in the first place
throw new LeaseTimeoutException( | ||
$"Timed out trying to acquire lease [{_leaseName}, {_settings.OwnerName}]. It may still be taken."); | ||
_acquireTask = _leaseActor.Ask(new LeaseActor.Acquire(leaseLostCallback), _timeout) | ||
.ContinueWith(t => |
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.
LGTM
Posible Fix for #1251
Changes