XComp commented on a change in pull request #18869:
URL: https://github.com/apache/flink/pull/18869#discussion_r813959119
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStore.java
##########
@@ -219,6 +242,11 @@ public void replace(String pathInZooKeeper,
IntegerResourceVersion expectedVersi
final String path = normalizePath(pathInZooKeeper);
+ checkState(
+ hasLock(path),
+ "'{}' is only allowed to be replaced if the instance has a
lock on this node.",
+ path);
Review comment:
That was added after investigating the code paths. Initially, I wanted
to make the `replace` call fail if the caller doesn't have a lock on that node.
This wouldn't happen because of the `get(path, false)` call below. I could have
replaced it by `getAndLock` which would set the lock if the caller didn't set
it, yet, or fail if the node is marked for deletion.
But that would have changed the semantics of the `replace` method slightly.
Instead, I went for hardening the implementation by adding this preconditions.
See [my comment](https://github.com/apache/flink/pull/18869/files#r813931306)
on the `hasLock` change thread below to see why the `replace` method is always
called after a lock is already acquired.
--
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]
