[lock] interoperate with Go client

[pmazzini]

Go client uses a different prefix.

Ref:

• Zookeeper python client cannot work with go client when using lock #548
• [lock] interoperate with Python client go-zookeeper/zk#19

[pmazzini]

I'll add a unittest.

[pmazzini]

Before:

While a Go client is holding a Read/Write lock the Python client would still be able to acquire a lock.

After:

While a Go client is holding a Read/Write lock the Python client properly waits for the lock to be released.

[ceache]

Thanks for the PR, looks good to me.

I think longer term, an enhancement would be to pass the pattern(s) to the lock constructor so that we do not have to do this again for Java, C# etc etc.
This can be put as an improvement for later on.

Question actually, do you know how the go client handles this? Does it watch for Python locks as well? Takes a list of patterns?

[pmazzini]

I think longer term, an enhancement would be to pass the pattern(s) to the lock constructor so that we do not have to do this again for Java, C# etc etc.
This can be put as an improvement for later on.

Sounds good.

Question actually, do you know how the go client handles this? Does it watch for Python locks as well? Takes a list of patterns?

I created a simple PR to watch for Python locks: go-zookeeper/zk#19.
No list of patterns for now but may be a future improvement as well.

[StephenSorriaux]

Hi,

Thanks for your time on this PR.

I agree with @ceache about passing a list of patterns to the Lock() constructor (using a new kwarg for instance). I was thinking about backward compatibility and I am not sure if merging this as if would generate some problems/side effets to our current user base. The long term solution seems safer. @pmazzini would you be willing to implement it? Any thoughts?

[pmazzini]

I was thinking about backward compatibility and I am not sure if merging this as if would generate some problems/side effets to our current user base.

It looks pretty safe to me. What kind of backward compatibility are you referring to?

Ideally all libraries should use the same prefix/protocol for acquiring locks. However, since that is not the case, this change only makes them be able to interoperate.

I agree with having a pattern in the future to make it more extensible. To make it easier to support other other libraries (Java, C#, etc) which may be doing something different but not for safety reasons. Shouldn't the default pattern be the one that allows the libraries to interoperate?

I would move forward with this change for now unless you see a problem with it.

[pmazzini]

ping

[ceache]

@pmazzini Would you be OK with this implementation?

[pmazzini]

Would you be OK with this implementation?

Thanks for taking it over. I would default it to be able to interoperate with the Go client. Other than that, it looks ok to me.

[StephenSorriaux]

I was thinking of any impact on the Semaphore() recipe seeing this comment but did not manage to find anything.

I am not sure why we should accept go lib pattern as default here? If we do that, we then would also need to do the same for any other lib... If the reason is "because this is how the official Zookeeper documentation recommends to implement a Lock(), then I'm perfectly fine with it.

(As a note, please follow our CONTRIBUTING.md guidelines for your commit)

[pmazzini]

If the reason is "because this is how the official Zookeeper documentation recommends to implement a Lock(), then I'm perfectly fine with it

Yeah, good point, the Go lib is following the official docs.

[ceache]

Ok, let me have a look at the official docs vs the go implementation and the java implementation of the locks. The locks names in the official docs are "path/<guid>-lock-<seq>" for regular locks. But for shared locks, they are "<guid>-/reader-<seq>" / "<guid>-/writer-<seq>" which does not make sense to me. If these are guides and not specs, then there will be interpretations and variations (like in this case).

Let me see if I can come up with something.

[pmazzini]

The Go client is only implementing regular locks following that doc.

Let me see if I can come up with something.

Any thoughts?

[ceache]

I have had a look at the Java client recipe, at another Python client (zkaio) and gozk.

It seems it is all a Zoo (pun intended): As far as I can tell, Java uses the session ID as guid, zkaio has different naming (no "-lock-"), go-zookeeper has some strange prefix and departs from it when it comes to r/w locks.
So, in summary, everyone has they own interpretation of these "official docs".

I then looked at our code more closely and found some interesting bugs/shortcomings such as #605 and, more importantly #606 (which seem to be shared by the other implementations, BTW).

Back to this issue, I have for now made a patch that renders our matching much more strict (only consider "{WORD}{SEQUENCE}$" as contenders) and that makes our matching much more safe, especially in a diverse environment with additional patterns.
So the option of having additional patterns will be a fully working alternative.

As for using go naming by default, I think we should decide as a whole to either a) switch completely to the "official" naming pattern and deprecate (but still support) the old format or b) stick to supporting alternate schemes as optional.
@python-zk/maintainers what do you think?

[ceache]

@StephenSorriaux @jeffwidman ping

Which way do you lean?

[StephenSorriaux]

Thank you a lot @ceache for digging in all of this. You're right, all of this is just a real Zoo, and I don't see any point to bother follow what the "official" documentation may recommend if the Java Zookeeper recipe does not even try to (x-sessionId- is used)...
In my opinion, and to answer your question, I would go with the option b) you mentioned since I don't see any gain switching to the other one. I really like the new extra_lock_patterns kwarg, along with the fixes you added.