kezhuw commented on code in PR #2064:
URL: https://github.com/apache/zookeeper/pull/2064#discussion_r1328506083
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java:
##########
@@ -735,10 +735,18 @@ private String getParentPathAndValidate(String path)
throws BadArgumentsExceptio
}
private static int checkAndIncVersion(int currentVersion, int
expectedVersion, String path) throws KeeperException.BadVersionException {
- if (expectedVersion != -1 && expectedVersion != currentVersion) {
- throw new KeeperException.BadVersionException(path);
+ if (expectedVersion != -1) {
+ if (expectedVersion != currentVersion) {
+ throw new KeeperException.BadVersionException(path);
+ }
+ if (expectedVersion == -2) {
+ return 0;
+ } else {
+ return currentVersion + 1;
+ }
+ } else {
+ return currentVersion + 1;
Review Comment:
Hmm, it is all about [`ZooKeeper::setData(String path, byte[] data, int
version)`](https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L2185).
Its doc states that:
> Set the data for the node of the given path if such a node exists and the
given version matches the version of the node (if the given version is -1, it
matches any node's versions). Return the stat of the node.
We normally use `setData` in two ways:
1. Call it with `-1` as `version` parameter to express an assignment.
2. Call it with `Stat.version` as `version` parameter to express a
conditional update.
So, `Stat.version` can't be `-1` as `-1` was reused to express an assignment.
> So my question is why should we skip this check when it is -2?
I recommend we change the commit message to something like "Skip
Stat.version value -1 to fix unconditional setData". All other values are
irrelevant and possibly misleading.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
