You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by "Hexiaoqiao (via GitHub)" <gi...@apache.org> on 2023/08/31 07:53:04 UTC

[GitHub] [curator] Hexiaoqiao opened a new pull request, #478: CURATOR-688. SharedCount will be never updated successful when version of ZNode is overflow.

Hexiaoqiao opened a new pull request, #478:
URL: https://github.com/apache/curator/pull/478

   I met one issue which will never update znode value successfully when integer overflow (-2147483648) of znode data version using curator to invoke SharedCount#trySetCount(VersionedValue<Integer>, int).
   After dig the limitation logic and found that here could be the root cause.
   
   https://github.com/apache/curator/blob/master/curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java#L196-L209
   
   My environment is, curator version: curator-2.10.0, zookeeper version: 3.4.6


-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1703855895

   @Hexiaoqiao Sorry for the delay. I am stuck about and also fearing this overflow things. There are not simple bugs, there are limitations. Hard to fix, only fragile workaround for best wish. I have some chaos thoughts for this issue:
   1. Curator guarantees no ordering-ness about  `VersionedValue.getVersion`. That is good. Better to document this out of order issue or make it private in future.
   2. The promise about "up-to-date" should be guaranteed.
   3. Silent "never update" is not good which is the goal for us to fix here.
   
   I am ok to a workaround but I am also think exception(e.g. let caller know they hit an implementation limitation) is a good fallback except for background watcher. Though, I am not positive to a good workaround. I think there are few choices when encountering this hard limitation.
   1. Push foward underlying implementation. In this case, a `check_zxid` version of `setData` in ZooKeeper.
   2. Rewrites something in caller side. In this case, either curator side or your application.
   3. Fragile workaround.
   
   All these suggestions happened in https://lists.apache.org/thread/4o3rl49rdj5y0134df922zgc8clyt86s.  cc @li4wang 
   
   Besides above, did you find this in production ? What is your use case ? @Hexiaoqiao 


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1700658027

   Some conclusion,
   a. zookeeper server side only add 1 for version when update znode. And it allows integer overflow.
   b. curator side can't be compatible with integer overflow because it compares old version and new version here and not consider the negative version number.
   https://github.com/apache/curator/blob/master/curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java#L196-L209
   c. The issue will be triggered from user interface both SharedCount#trySetCount and SharedCount#start(). When znode version meet Integer.MIN_VALUE SharedCount will be never update because condition `current.getVersion() >= version` always true.
   d. While add condition `current.getVersion() >= version && version != Integer.MIN_VALUE`, this case could be resolved. And update znode as expected.


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1700545478

   Sorry without new unit test here. Because I want to init one znode with version Integer.MAX_VALUE but no interface found. It will be helpful if someone give some guide to cover this case.


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311264108


##########
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:
   Thanks @eolivelli , we try to build tmp zookeeper version and create zonde with version Integer.MAX_VALUE force and this case can reappear when trySetCount this znode. When meet this issue, SharedCount#trySetCount(VersionedValue<java.lang.Integer>, int) will always return false, then application will be stuck.



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1722674942

   @tisonkun Glad to see you here.
   
   > While it can be better for ZK to jump from version -2 to 0 so that in an overflow loop we don't handle -1 hole, the restart and get versioned value = -1 remains and set data concurrently doesn't hurt as long as we read the updated data finally.
   
   It will be more smooth while ZK to skip version -1, Strong +1. From my first thought, it would be the next step after we fix at Curator side. Now it is great to see that other guys already try to push this forward. 
   For ZOOKEEPER-4743, I just suggest that we should consider to solve dataVersion/aclVersion/cversion 32-integer overflow at once.
   Back to this PR, what should we do before ZOOKEEPRER-4743 check in, just wait ZOOKEEPER-4743 to be ready or other ways? Thanks.


-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1717815870

   >  'we have to throw exception if previous.getVersion() equals to -1 in call chain of trySetCount', Will it never update successfully when dataversion is back to -1 if we need to guarantee this rule?
   
   Yes, and no. When `previous.getVersion` reach to `-1`, `trySetCount` has few choices:
   1. Exception to caller for limitation of the implementation, that is `Stat.version` overflowed and wraparound to `-1` and [`setData` with `-1` `version`](https://github.com/apache/zookeeper/blob/b31f776471fef79ab161f416d58367bdffaf37a9/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L2187) is a blind update.
   2. Keep silent and do a blind update.
   
   The later behavior is a bug due to the contract what `trySetCount` try to express. As a library, I don't think Curator should do that for callers silently.
   
   For the "no" part, callers can risk themselves by doing a blind update through `setCount`.


-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1721316198

   > While it can be better for ZK to jump from version -2 to 0 so that in an overflow loop we don't handle -1 hole, the restart and get versioned value = -1 remains and set data concurrently doesn't hurt as long as we read the updated data finally.
   
   Good idea! I just saw [ZOOKEEPER-4743](https://issues.apache.org/jira/browse/ZOOKEEPER-4743). 


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311273506


##########
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:
   Addendum: I still try to dig if it meet wrong version before updateValue.



-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1704450591

   @Hexiaoqiao Thanks your sharing, that is important!
   
   I found some potential problem in the usages, though I haven't take a deep in look. `incrSharedCount` uses batch which is what Ted Dunning suggested in above mail thread. That is good. But it is somewhat error-prone in implementation.
   1. The counter itself is a 32-bit integer, so it will overflow finally regard less of `Stat.version`.
   2. The default `batchSize` is small. I got two defaults. `public static final int DEFAULT_SEQ_NUM_BATCH_SIZE = 10;` and `public static final int ZK_DTSM_TOKEN_SEQNUM_BATCH_SIZE_DEFAULT = 1;`.
   
   So, for your use case, I suggest:
   1. Rewrites sequence number as 64-bit integer. I am not sure what the consequence of this. But 32-bit in your case will overflow finally. I could overflowed already hiding by overflow of `Stat.version`.
   2. Uses a sensible default batch size and refuse small batch size. This is easy in your case, but you will get negative sequence number cause of above.
   
   `Stat.version` is 32-bit, it is not good to do a 1 to 1 mapping to application sequence number.


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1717637752

   Thanks @kezhuw for your suggestions and sorry for the late response.
   
   > The promise about "up-to-date" should be guaranteed.
   > The promise about "Changes the shared value only if its value has not changed since the version specified by newValue" should be hold. This means we have to throw exception if previous.getVersion() equals to -1 in call chain of trySetCount.
   
   I am not sure if I understood this point clearly. 'we have to throw exception if previous.getVersion() equals to -1 in call chain of trySetCount', Will it never update successfully when dataversion is back to -1 if we need to guarantee this rule?


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1700975264

   One corner case didn't considered, and try to update this PR and trigger CI again.


-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1342114575


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java:
##########
@@ -196,8 +196,12 @@ 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) {
-                // A newer version was concurrently set.

Review Comment:
   I have pushed new commits to go through above direction. Could you please take a look @Hexiaoqiao @tisonkun @eolivelli ?



-- 
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


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

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1720606842

   > TBH, I am not familiar with implement about Curator, so would you mind to list some demo code snippet about configuration if this solution could be approved.
   
   Somehow you can copy the shared count implementation..It's not quite a lot of code.
   
   Since this requirement is quite customized, I'm afraid that it's not suitable to hack into Curator. The root cause is integer overflow that is backed by ZK. 
   
   Also, I don't understand actually how this patch "fixes" the issue. You add more strict condition to exit but the original issue is hang?


-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1327946265


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java:
##########
@@ -196,8 +196,12 @@ 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) {
-                // A newer version was concurrently set.

Review Comment:
   I think we should refactor `updateValue` a bit to compare ordering using `Stat.mzxid`. This way we are not fearing this overflow issue. I am stupid in reviwing without a deep thought, sorry for that. So my finally points are:
   * Deprecate `VersionedValue#getVersion` to warn clients about "ordering assumptions" and "overflow behavior". 
   * Refactor `updateValue` to order using `Stat.mzxid`.
   * Throw exception in case of `-1` `Stat.version` in `trySetValue`. I am positive to [ZOOKEEPER-4743](https://issues.apache.org/jira/browse/ZOOKEEPER-4743).
   * Document somehow about "overflow" and exception case in `trySetValue`.
   
   Besides above, should we expose a `VersionedValue#getZxid` for client usage ?
   
   Any thoughts @tisonkun @eolivelli @Hexiaoqiao ?



-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1725540750

   > what should we do before ZOOKEEPRER-4743 check in, just wait ZOOKEEPER-4743 to be ready or other ways?
   
   I suggest we use `Stat.mzxid` to guard ordering in `updateValue`. (I posted in another thread https://github.com/apache/curator/pull/478/files#r1327946265).
   
   The current approach does not sound correct to me(https://github.com/apache/curator/pull/478/files#r1313839760). And I think it is error-prone.
   
   When `Stat.version` is `-1`, exception is enough for us to go. ZOOKEEPRER-4743 is another story, Curator should work irrespective of ZooKeeper version.


-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1737014906

   Hi @Hexiaoqiao, do you still work on this ? Or should I take over this ? I plan to resort to `Stat.mzxid` in `updateValue`.


-- 
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


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

Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311247910


##########
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:
   what happens after we reach this new condition ?
   is the system stuck ? 
   
   also, as "version" is a constant here,  could early exit ?  in the beginning of the method
   



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311396132


##########
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:
   > But when current.getVersion() reach -1, the next trySetCount is indeterminate.
   
   Sorry, I don't get this meaning, the server side indead to check if version is -1. But if both `version` and `currentVersion` are -1, it will increase as expect. Do you mean that some other logic I missed at curator side? Thanks.



-- 
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


[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.

Posted by "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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1326864282


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java:
##########
@@ -196,8 +196,12 @@ 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) {
-                // A newer version was concurrently set.

Review Comment:
   I am not convinced in the overflow case either. @tisonkun @Hexiaoqiao 
   
   For the overflow case,
   * +1 to deprecate `VersionedValue#getVersion` so to warn clients about the "ordering assumption" if any about `version`.
   * +1 to a viable workaround if any and/or exception.



-- 
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


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

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1720906809

   I understand the condition now and agree that this patch should work.
   
   While it can be better for ZK to jump from version -2 to 0 so that in an overflow loop we don't handle -1 hole, the restart and get versioned value = -1 remains and set data concurrently doesn't hurt as long as we read the updated data finally.


-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1313839760


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/shared/SharedValue.java:
##########
@@ -196,8 +196,12 @@ 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) {
-                // A newer version was concurrently set.

Review Comment:
   I am not sure wether `SharedValue` was designed to work with multiple owners, but I saw there is a background watcher to update value and also there is no rule to forbid concurrent usages. So, I assume it should work well in case of concurrency.
   
   Then, let me assume a situation:
   1. `current.getVersion` is `Integer.MAX_VALUE`.
   2. Thread1 call `trySetValue` and succeed to get overflowed version `Integer.MIN_VALUE`, but the call to `updateValue` is somewhat delayed.
   3. Thread2 (assume watcher, which runs in ZooKeeper thread if I am not wrong) call `updateVersion` with version `Integer.MIN_VALUE + 1`. According to the code, this will be ignored.
   4. Thread1 call `updateValue` to continue its task with version `Integer.MIN_VALUE`. It succeeds.
   5. That is all, assume no changes anymore. I know it may not realistic.
   
   `current` stores dated version while the javadoc says "All clients watching the same path will have the up-to-date value (considering ZK's normal consistency guarantees)".



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311361585


##########
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:
   > I really hope a check_zxid style setData.
   
   Totally +1. It should push zookeeper to changes, which is another thing. If need we should file another thread to discuss.
   
   Addendum, here I didn't traverse cversion and aclversion if also have the same issue at curator side.



-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311408914


##########
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:
   It is [checkAndIncVersion](https://github.com/apache/zookeeper/blob/03a36d08e257c43e8377e5549d5524805fc6b8bb/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L737) in ZooKeeper side. When the `expectedVersion` is `-1`, it means `setData` blindly.
   
   In Curator side, that is when `getVersionedValue` reports `-1`, the next `trySetValue` will ship `-1` as `expectedVersion` to server which is apparently not what we want.



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1702528697

   Hi @eolivelli @kezhuw , anymore suggestions here?


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1704541547

   @kezhuw Great response. Some concerns here.
   
   > * Rewrites sequence number as 64-bit integer. I am not sure what the consequence of this. But 32-bit in your case will overflow finally. It could overflowed already hiding by overflow of `Stat.version`.
   > * Uses a sensible default batch size and refuse small batch size. This is easy in your case, but you will get negative sequence number cause of above.
   
   Actually sequence number can be compatible with 32-bit integer overflow, which only should be distinguishable number without other conditions. IMO it is OK even if overflow, actually I try to test it on our test env and it works fine. So it is not hiding issue here.
   Another side, I totally agree that set the default configuration `ZK_DTSM_TOKEN_SEQNUM_BATCH_SIZE_DEFAULT` to some bigger number (such as 10 or much more) could bypass this issue, however it does not solve root cause about Curator. Right?
   Thanks again.


-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1313840755


##########
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:
   I guess `-1` was chose as `UNINITIALIZED_VERSION` because of it is minimum in absent of overflow. So, now we can have `Integer.MIN_VALUE` as `UNINITIALIZED_VERSION` 😮‍💨 .



##########
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:
   I guess `-1` was chose as `UNINITIALIZED_VERSION` because of it is minimum in absent of overflow. So, now we can have `Integer.MIN_VALUE` as `UNINITIALIZED_VERSION` 😮‍💨 .



-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1720749918

   > So how about expose the choice to end user and offer some configuration items to set? IMO, some sensitive data should not be updated blindly, but some case we should offer solution to bypass this bug
   
   I am -0 to this approach. Exception is the default, in anyway. Clients can bypass themselves easily on exception if they encounter or aware of this. I don't think it is worth for Curator to do that for this limitation(either ZooKeepr or Curator implementation from perspective) in case of awareness from clients. The important things for Curator here from my side are documentation and exception in code. Anything beyond that are probably overkill. 


-- 
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


Re: [PR] CURATOR-688. SharedCount will be never updated successful when version of ZNode is overflow. [curator]

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1752264691

   Sorry for the late response since I took a long vacation. Please feel free to take over it if interested.


-- 
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


Re: [PR] CURATOR-688. SharedCount will be never updated successful when version of ZNode is overflow. [curator]

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1758881797

   Thank you @Hexiaoqiao ! I have pushed a commit to using `Stat.mzxid`. Would you mind to take a look ?


-- 
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


[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.

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311375125


##########
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:
   Sound reasonable circumvent. But when `current.getVersion()` reach `-1`, the next `trySetCount` is indeterminate.
   
   And absolutely, we should document a bit, either this wraparound and indeterminate in `-1` or old behavior. I even think whether an exception is more appropriate ?



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311553460


##########
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:
   One corner case when review code back, if the version overflow at server side and it is negative value such as -100 now, then restart application and `currentValue` will be initialized at curator side, and the version number will be set to UNINITIALIZED_VERSION which is -1 now. Then SharedCount will never be updated because `current.getVersion() >= version && version != Integer.MIN_VALUE` always true now.
   cc @kezhuw @eolivelli What do you think about?



-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1702022652

   fix checkstyle and trigger ci again.


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on code in PR #478:
URL: https://github.com/apache/curator/pull/478#discussion_r1311416280


##########
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:
   Yes, my bad. We both say the same code segment, but I misunderstand it. I will add some javadoc for this changes moment later. Thanks again.



-- 
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


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

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1720914681

   > reproduce the problem
   
   @eolivelli The tricky point here is that ZK doesn't expose API to manually edit node version so you should change the data for Int.MAX times which can be time-consuming..


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1718708611

   > The later behavior is a bug due to the contract what trySetCount try to express. As a library, I don't think Curator should do that for callers silently.
   
   +1, Agree. So how about expose the choice to end user and offer some configuration items to set? IMO, some sensitive data should not be updated blindly, but some case we should offer solution to bypass this bug, such as `Token for Hadoop` as mentioned above, which can update smoothly even blindly. What do you think about?
   TBH, I am not familiar with implement about Curator, so would you mind to list some demo code snippet about configuration if this solution could be approved.


-- 
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


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

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1728162574

   > Back to this PR, what should we do before ZOOKEEPRER-4743 check in, just wait ZOOKEEPER-4743 to be ready or other ways? Thanks.
   
   They are irrelevant things.
   
   For this patch, I may give another look as well as the comments above. @Hexiaoqiao you can try to address @kezhuw's comments/


-- 
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


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

Posted by "Hexiaoqiao (via GitHub)" <gi...@apache.org>.
Hexiaoqiao commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1704329621

   @kezhuw Thanks for your detailed comments. I totally agree that this issue is not very easy/simple to fix perfectly. But for my case, this improvement could fix it when try to reproduce.
   
   > Besides above, did you find this in production ? What is your use case ?
   
   Of course YES. The corresponding code snippet as following shows.
   
   https://github.com/apache/hadoop/blob/a6c2526c6c7ccfc4483f58695aedf0cb892b4c4d/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java#L506-L516
   
   I would like to give a brief explanation (which is dependent by Hadoop project).
   A. For HDFS RBF architecture[1], all Routers share the same states which is managed by zookeeper, and Token is one of these states.
   B. About token[2], one Router generate token using increment sequence number shared by all Routers and update corresponding znode value (/zkdtsm/ZKDTSMRoot/ZKDTSMSeqNumRoot), let's name it Z.
   C. Where token number will be million/day or more, and the version of znode Z will overflow only years after create(maybe less than three years if 5 million/day tokens generated.)
   
   At first, I want to fix it at Hadoop side, but it is not smoothy and could not fix the root cause.
   
   For this PR, my first thought is fix at curator side first, then try to improve it at both zookeeper and curator side as the solution you mentioned above. Any thoughts? Thanks.
   
   [1] https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-hdfs-rbf/HDFSRouterFederation.html
   [2] http://hortonworks.com/wp-content/uploads/2011/08/adding_security_to_apache_hadoop.pdf


-- 
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


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

Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #478:
URL: https://github.com/apache/curator/pull/478#issuecomment-1704716440

   > IMO it is OK even if overflow, actually I try to test it on our test env and it works fine. So it is not hiding issue here.
   
   @Hexiaoqiao  Glad to hear, that is good!
   
   > however it does not solve root cause about Curator. Right?
   
   I am open to a workaround as long as it fit. For a workaround to work, I think we should guarantee:
   1. The promise about "up-to-date" should be guaranteed.
   2. The promise about "Changes the shared value only if its value has not changed since the version specified by newValue" should be hold. This means we have to throw exception if `previous.getVersion()` equals to `-1` in call chain of `trySetCount`.
   
   I feel hopeless about viable workaround, but not against one. Anyway please go ahead, I would be happy to hear good news.


-- 
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