Sam Baskinger commented on ZOOKEEPER-767:

Thanks for the feedback Benjamin,

Replying by email removed snippets from your message. Same comments as above, 
but with quotes for context (and fewer smilies).

Perhaps I misread the recipe or am missing the philosophy of ZK's atomicity. It 
wouldn't be the first time. To your points:

> 1) shouldn't you check to see if you already have a lock before you do the 
> create? that will remove the code right after the create in the getXXXXLock() 
> methods.

We do the create to ensure that we, at some point, will hold a lock. I do want 
to do the create, ensuring my turn, and then wait until I'm at the front of the 
line (front being defined in the exclusive or shared way).

> 2) if you already have an exclusive lock, shouldn't that also count as a 
> shared lock?

There should be a unit test that ensure that this does indeed happen, 
semantically. Exclusive locks block all shared access, if I take your meaning 

> 3) the error handling is a bit problematic. a connection loss exception or an 
> interrupt can leave a process holding a lock without knowing it.

I thought the API guaranteed that in the event of a connection loss the 
EPHEMERAL creation property would guarantee that when the session timed out the 
file would be removed and watchers would be signaled.

> 4) when you go through the children, you may end up checking for the 
> existence of every znode before you, which could be wasteful.

All but those behind me in the line of locks. This could certainly be optimized 
and is something I thought about, but moved past to get the rough 
implementation in flight.

> i think it may be better to expand the current locking code to handle shared 
> lock rather than add a new lock implementation. the current lock recipe
> implementation only does exclusive locks, but it is implemented in a way that 
> makes it easy to support shared locks as well and it takes care of the
> above problems.

If the above 4 points hold, then extending the other implementation may be 
better for the community.  I hope you'll include the code, but if not, we're 
very happy with it and appreciate ZooKeeper! Keep up the fine work. 

What do you think? What did I miss? :)

Sam Baskinger

> Submitting Demo/Recipe Shared / Exclusive Lock Code
> ---------------------------------------------------
>                 Key: ZOOKEEPER-767
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-767
>             Project: Zookeeper
>          Issue Type: Improvement
>          Components: recipes
>    Affects Versions: 3.3.0
>            Reporter: Sam Baskinger
>            Assignee: Sam Baskinger
>            Priority: Minor
>             Fix For: 3.4.0
>         Attachments: ZOOKEEPER-767.patch, ZOOKEEPER-767.patch
> Networked Insights would like to share-back some code for shared/exclusive 
> locking that we are using in our labs.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to