-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implements lock watching and lock pouncing #17
Conversation
… when caches are inexistent
… moves Acquire to acquire while allowing Acquire to call Pounce directly
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.
Additional comments :
Remove the internal/lockclient/logs
file
Questions:
Is pounce really dormant if it is being used to implement Acquire()
?
I've reviewed up to Acquire()
in the simple lock client file. I'll review the pounce code tomorrow
@@ -50,10 +54,14 @@ func NewLRUCache(capacity int) *LRUCache { | |||
// | |||
// The element is removed from the map too because | |||
// it might have stale node values. | |||
func (lru *LRUCache) GetElement(element interface{}) error { | |||
// | |||
// Error is returned only if the element doesn't exist in the cache. |
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.
Specify that the owner of the item is being returned by the function
Currently, it says 'GetElement gets an element from the cache'
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.
Maybe we shouldnt be so blunt about it being the owner
, it can technically be anything in the future. Ill mention, it returns associated data will be returned along with an error.
// The client has the ability to start the lockservice from its | ||
// in-built function or it can be started separately. | ||
// | ||
// The client offers the user to Acquire a lock, Release a lock, |
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.
does ' the client offers the functional of Acquiring and Releasing an object using its descriptor' sound better?
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.
Ill go with the client allows the user
.
// file is NOT acquired. | ||
func (sc *SimpleClient) getFromCache(d lockservice.ObjectDescriptor) (string, error) { | ||
if sc.cache != nil { | ||
owner, err := sc.cache.GetElement(cache.NewSimpleKey(d.ObjectID, "")) |
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.
If GetElement()
is returning the owner, we should change it such that passing ""
to it as a parameter for the owner of the element isn't needed. What do you think?
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.
Agreed, I think this was a quick fix by me. Maybe I should create a new object, Ill see what I can do.
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.
Actually, these are my options:
- Create another function that does it (overloading is not allowed so yeah new function with a different sign is needed)
- Pass
&SimpleKey{d.ObjectID}
So the current way seemed to be the most clean one.
This PR resolves #15, lock watching.
This PR had to have some major architectural changes with respect to moving
Acquire
toacquire
and the exposedAcquire
callingPounce
in order to maintain the order of lock acquisition.It also moves
SimpleDescriptor
toLockDescriptor
and introduces a newObjectDescriptor
thus resolving #21 .