Adds configurable leases to locks to support lock expiry #34
Conversation
SUMUKHA-PK
left a comment
SUMUKHA-PK
left a comment
There was a problem hiding this comment.
Preliminary clean up review, will do a functional review after this!
Good job on getting the PR done!
internal/lockclient/simple_client.go
Outdated
| time.Sleep(200 * time.Millisecond) | ||
| // changed to 10s just to test lock expiry | ||
| // TODO: Make the desired length of session expiry user configurable | ||
| time.Sleep(10000 * time.Millisecond) |
There was a problem hiding this comment.
And? Making it a configurable, injectable parameter.
|
|
||
| got = sc.Release(d, session) | ||
| want = ErrSessionNonExistent | ||
| want = lockservice.ErrCantReleaseFile |
There was a problem hiding this comment.
So i changed the test that checks if session expiry works to instead check for lock expiry.
This test highlighted the redundancy for either lock expiry / session expiry. I feel like only one of them is needed. Having both is a bit redundant imo. What do you think @SUMUKHA-PK ?
|
|
||
| // If the lock is not present in the LockMap or | ||
| // the lock has expired, then allow the acquisition | ||
| if _, ok := ls.lockMap.LockMap[sd.ID()]; !ok || (ok && (compareDuration(ls.lockMap.LockMap[sd.ID()].timestamp, ls.lockMap.LeaseDuration))) { |
There was a problem hiding this comment.
So do this instead of this messy thing:
conditioName := ok && (compareDuration(ls.lockMap.LockMap[sd.ID()].timestamp, ls.lockMap.LeaseDuration)) (name condition1 appropriately)
Then combine them.
| Str("owner", ls.lockMap.LockMap[sd.ID()].owner). | ||
| Time("timestamp", ls.lockMap.LockMap[sd.ID()].timestamp). | ||
| Msg("locked") | ||
| return nil |
There was a problem hiding this comment.
the if condition has changed. The new if condition is essential NOT of the previous one. If you see the additions at like #188, it contains what has been removed here.
| } else if _, ok := ls.lockMap.LockMap[sd.ID()]; !ok { | ||
| return ErrCantReleaseFile | ||
|
|
||
| } else if compareDuration(ls.lockMap.LockMap[sd.ID()].timestamp, ls.lockMap.LeaseDuration) { |
There was a problem hiding this comment.
Is there a less messy way of doing this?
There was a problem hiding this comment.
is this really messy? Isn't it just a function call with current timestamp and lease duration as parameters?
Co-authored-by: Sumukha Pk <sumukhapk46@gmail.com>
internal/lockclient/simple_client.go
Outdated
| // NewSimpleClient returns a new SimpleClient of the given parameters. | ||
| // This client works with or without the existance of a cache. | ||
| func NewSimpleClient(config *lockservice.SimpleConfig, log zerolog.Logger, cache *cache.LRUCache) *SimpleClient { | ||
| func NewSimpleClient(config *lockservice.SimpleConfig, log zerolog.Logger, cache *cache.LRUCache, sessionDuration time.Duration) *SimpleClient { |
There was a problem hiding this comment.
This line is too long, try cascading them to lines
| func NewSimpleClient(config *lockservice.SimpleConfig, log zerolog.Logger, cache *cache.LRUCache, sessionDuration time.Duration) *SimpleClient { | |
| func NewSimpleClient(config *lockservice.SimpleConfig, | |
| log zerolog.Logger, | |
| cache *cache.LRUCache, | |
| sessionDuration time.Duration | |
| ) *SimpleClient { |
Something like this with lint ^
internal/lockclient/simple_client.go
Outdated
| sc.mu.Unlock() | ||
| // Sessions last for 200ms. | ||
| time.Sleep(200 * time.Millisecond) | ||
| // Sessions last for user configured duration |
There was a problem hiding this comment.
| // Sessions last for user configured duration | |
| // Sessions last for user configured duration. |
|
|
||
| got = sc.Release(d, session) | ||
| want = ErrSessionNonExistent | ||
| want = lockservice.ErrCantReleaseFile |
| func NewSimpleLockService(log zerolog.Logger, leaseDuration time.Duration) *SimpleLockService { | ||
| safeLockMap := &SafeLockMap{ | ||
| LockMap: make(map[string]string), | ||
| LockMap: make(map[string]*LockMapEntry), |
There was a problem hiding this comment.
Does this have to be a pointer?
| func hasLeaseExpired(timestamp time.Time, lease time.Duration) bool { | ||
| intDuration := int64(time.Now().Sub(timestamp)) | ||
| intLease := int64(lease) | ||
| fmt.Printf("%d %d \n", intDuration, intLease) |
| } | ||
|
|
||
| // hasLeaseExpired returns true if the lease for a lock has expired and | ||
| // returns false if the lease is still valid |
There was a problem hiding this comment.
| // returns false if the lease is still valid | |
| // false if the lease is still valid. |
|
|
||
| // If the lock is not present in the LockMap or | ||
| // the lock has expired, then allow the acquisition | ||
| if _, ok := ls.lockMap.LockMap[sd.ID()]; !ok || hasLeaseExpired(ls.lockMap.LockMap[sd.ID()].timestamp, ls.lockMap.LeaseDuration) { |
There was a problem hiding this comment.
Can this be made less messy?
Say have a timestamp := ls.lockMap.LockMap[sd.ID()].timestamp and this use it while returning too?
| type SafeLockMap struct { | ||
| LockMap map[string]string | ||
| Mutex sync.Mutex | ||
| LockMap map[string]*LockMapEntry |
There was a problem hiding this comment.
SafeLockMap.LockMap seems weird.
Make the variable as map or something that doesnt repeat weirdly. (Kinda a Go standard)
When a lock server is created, a lease duration has to be set. All locks that are granted are only valid for this specified duration.
A client can acquire a lock only if it has expired or if it has been released.
A lock can be released only by the session that acquired it and only if its lease is still valid.
@SUMUKHA-PK