Fix parseSeq incorrectly parsing negative sequence number

[colinmcintosh]

This PR includes a change to the parseSeq function used by the lock to parse sequence numbers. When the Zookeeper sequence number rolls over it rolls to -2147483647 rather than 0. parseSeq splits on a "-" symbol so when the path is split into parts the negative symbol is stripped away incorrectly. I changed this to split on the full prefix of "lock-" instead which saves the negative symbol. A test for this is included.

[pmazzini]

From the ZK docs:

the counter used to store the next sequence number is a signed int (4bytes) maintained by the parent node, the counter will overflow when incremented beyond 2147483647 (resulting in a name "< path >-2147483647").

[codecov]

Codecov Report

Merging #65 (c1e98b7) into master (8866d76) will increase coverage by 1.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   75.27%   76.28%   +1.00%     
==========================================
  Files           7        7              
  Lines        1189     1189              
==========================================
+ Hits          895      907      +12     
+ Misses        202      193       -9     
+ Partials       92       89       -3     

Impacted Files	Coverage Δ
lock.go	63.38% <100.00%> (ø)
conn.go	73.46% <0.00%> (+1.88%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8866d76...c1e98b7. Read the comment docs.

[pmazzini]

lgtm

[colinmcintosh]

Thanks for that link to the ZK docs @pmazzini! After I encountered this I searched for details in the docs but hadn't come across that paragraph.

[nemith]

Another option would just do a SplitN(..., 1) to only split on the first -

[colinmcintosh]

I think SplitN wouldn't work for the lock paths because CreateProtectedEphemeralSequential adds a "-" to beginning of the lock node path. I think it would probably also encounter an issue if a "-" is present in the parent path which also gets passed to parseSeq.

re request review

[pmazzini]

What I am confused about is that the Java implementation does not look to be checking negative numbers:
https://github.com/apache/zookeeper/blob/master/zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java#L54

[colinmcintosh]

Interesting, that does seem to be a bug in the Java implementation as well. I've open an issue in the Apache JIRA regarding that: https://issues.apache.org/jira/browse/ZOOKEEPER-4372

[colinmcintosh]

I submitted a PR for the Java implementation as well: apache/zookeeper#1751

[nemith]

Seems good to me, @pmazzini any thoughts or good to merge?

[pmazzini]

Looks good to me

[colinmcintosh]

Hi, just wanted to see if we could get this merged in as it sounds like there were no major concerns with this PR. Thanks!