[GitHub] [curator] kezhuw commented on a diff in pull request #478: CURATOR-688. SharedCount will be never updated successful when version of ZNode is overflow.

["kezhuw (via GitHub)" <gi...@apache.org>]

kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311342375


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java:
##########
@@ -196,7 +196,7 @@ public boolean trySetValue(VersionedValue<byte[]> previous, byte[] newValue) thr
     private void updateValue(int version, byte[] bytes) {
         while (true) {
             VersionedValue<byte[]> current = currentValue.get();
-            if (current.getVersion() >= version) {
+            if (current.getVersion() >= version && version != Integer.MIN_VALUE) {

Review Comment:
   Seems that [`VersionedValue.version`](https://github.com/apache/curator/blob/72beebc504d90338ded9a68ae0f6b79f5c8986fd/curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/VersionedValue.java#L27) are `Stat.version` [from ZooKeeper](https://github.com/apache/curator/blob/72beebc504d90338ded9a68ae0f6b79f5c8986fd/curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java#L251). Then it is "known" to overflow in finite time in possible large `setData` frequency. Given `default` visibility of `VersionedValue`'s visibility, I think we probably can enhance this a bit with extra `zxid`. But the crucial problem comes from ZooKeeper, it only do atomic check against `Stat.version` which is a 32-bit integer and overflow finally for long running frequently modified data. And we will finally run into corner case [`checkAndIncVersion`](https://github.com/apache/zookeeper/blob/03a36d08e257c43e8377e5549d5524805fc6b8bb/zookeeper-server/src/main/j
 ava/org/apache/zookeeper/server/PrepRequestProcessor.java#L737) exposed, `-1` is not an atomic condition which means we could write wrong node data in case of wraparound and contention.
   
   I really hope a `check_zxid` style `setData`. I am always fearing of `setData` with `version` from old incarnation.
   
   There is a similar case in https://lists.apache.org/thread/4o3rl49rdj5y0134df922zgc8clyt86s, which overflow `Stat.cversion`. I believed ZooKeeper was not designed and suitable for certain usages, performance degradation is desireable in wrong usages. But these overflow more sounds like bugs.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org