qqu0127 commented on code in PR #2432:
URL: https://github.com/apache/helix/pull/2432#discussion_r1169180845
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -384,6 +405,21 @@ public void unsubscribeStateChanges(
}
public void unsubscribeAll() {
+ if (_usePersistWatcher) {
+ ManipulateListener removeAllListeners = (String, Object) -> {
+ Set<String> paths = new HashSet<>();
+ _childListener.forEach((k, v) -> paths.add(k));
+ _dataListener.forEach((k, v) -> paths.add(k));
+ paths.forEach(p -> {
+ try {
+ getConnection().removeWatches(p, this, WatcherType.Any);
+ } catch (InterruptedException | KeeperException e) {
+ LOG.info("Failed to remove persistent watcher for {} ", p, e);
+ }
+ });
+ };
Review Comment:
This looks like BiFunciton out of box,
https://docs.oracle.com/javase/8/docs/api/java/util/function/BiFunction.html,
can we use it by any chance?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2927,4 +2990,54 @@ private void removeChildListener(String path,
IZkChildListener listener) {
listeners.remove(listener);
}
}
+
+ interface ManipulateListener<T> {
+ void run(String path, Object listener) throws KeeperException,
InterruptedException;
+ }
+
+ private void addPersistListener(String path, Object listener) {
+ ManipulateListener addListeners = (String, Object) -> {
+ if (listener instanceof IZkChildListener) {
+ addChildListener(path, (IZkChildListener) listener);
+ } else if (listener instanceof IZkDataListener) {
+ addDataListener(path, (IZkDataListener) listener);
+ }
+ };
+ executeWithInPersistListenerMutex(addListeners, path, listener);
+ }
+
+ private void removePersistListener(String path, Object listener) {
+
+ ManipulateListener removeListeners = (String, Object) -> {
+ try {
+ if (listener instanceof IZkChildListener) {
+ removeChildListener(path, (IZkChildListener) listener);
+ } else if (listener instanceof IZkDataListener) {
+ removeDataListener(path, (IZkDataListener) listener);
+ }
+ if (!hasListeners(path)) {
+ // TODO: update hasListeners logic when recursive persist listener
is added
+ getConnection().removeWatches(path, this, WatcherType.Any);
+ }
+ } catch (KeeperException.NoWatcherException e) {
+ LOG.warn("Persist watcher is already removed");
+ }
+ };
+
+ executeWithInPersistListenerMutex(removeListeners, path, listener);
+ }
+
+ private void executeWithInPersistListenerMutex(ManipulateListener runnable,
String path,
+ Object listener) {
Review Comment:
Not an issue, but thought on improving the raw Object class type:
what if we create an empty interface and let the two listeners interface
extend that as marker interface? It only looks slightly better.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2927,4 +2990,54 @@ private void removeChildListener(String path,
IZkChildListener listener) {
listeners.remove(listener);
}
}
+
+ interface ManipulateListener<T> {
+ void run(String path, Object listener) throws KeeperException,
InterruptedException;
+ }
+
+ private void addPersistListener(String path, Object listener) {
+ ManipulateListener addListeners = (String, Object) -> {
+ if (listener instanceof IZkChildListener) {
+ addChildListener(path, (IZkChildListener) listener);
+ } else if (listener instanceof IZkDataListener) {
+ addDataListener(path, (IZkDataListener) listener);
+ }
+ };
+ executeWithInPersistListenerMutex(addListeners, path, listener);
+ }
+
+ private void removePersistListener(String path, Object listener) {
+
+ ManipulateListener removeListeners = (String, Object) -> {
+ try {
+ if (listener instanceof IZkChildListener) {
+ removeChildListener(path, (IZkChildListener) listener);
+ } else if (listener instanceof IZkDataListener) {
+ removeDataListener(path, (IZkDataListener) listener);
+ }
+ if (!hasListeners(path)) {
+ // TODO: update hasListeners logic when recursive persist listener
is added
+ getConnection().removeWatches(path, this, WatcherType.Any);
+ }
+ } catch (KeeperException.NoWatcherException e) {
+ LOG.warn("Persist watcher is already removed");
+ }
+ };
+
+ executeWithInPersistListenerMutex(removeListeners, path, listener);
+ }
+
+ private void executeWithInPersistListenerMutex(ManipulateListener runnable,
String path,
+ Object listener) {
+ try {
+ _persistListenerMutex.lockInterruptibly();
+ runnable.run(path, listener);
Review Comment:
Similar concern as brought up above. Correct me if I'm wrong, I'm afraid the
input (path, listener) here isn't actually being used.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -384,6 +405,21 @@ public void unsubscribeStateChanges(
}
public void unsubscribeAll() {
+ if (_usePersistWatcher) {
+ ManipulateListener removeAllListeners = (String, Object) -> {
+ Set<String> paths = new HashSet<>();
+ _childListener.forEach((k, v) -> paths.add(k));
+ _dataListener.forEach((k, v) -> paths.add(k));
+ paths.forEach(p -> {
+ try {
+ getConnection().removeWatches(p, this, WatcherType.Any);
+ } catch (InterruptedException | KeeperException e) {
+ LOG.info("Failed to remove persistent watcher for {} ", p, e);
+ }
+ });
+ };
+ executeWithInPersistListenerMutex(removeAllListeners, null, null);
+ } else {
synchronized (_childListener) {
_childListener.clear();
Review Comment:
nit: you can just return after the first "if", so no need to do "else". Same
for below.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2927,4 +2990,54 @@ private void removeChildListener(String path,
IZkChildListener listener) {
listeners.remove(listener);
}
}
+
+ interface ManipulateListener<T> {
+ void run(String path, Object listener) throws KeeperException,
InterruptedException;
+ }
+
+ private void addPersistListener(String path, Object listener) {
+ ManipulateListener addListeners = (String, Object) -> {
+ if (listener instanceof IZkChildListener) {
+ addChildListener(path, (IZkChildListener) listener);
+ } else if (listener instanceof IZkDataListener) {
+ addDataListener(path, (IZkDataListener) listener);
+ }
+ };
Review Comment:
Hmmm, I'm so confused by the syntax.
On one hand you are defining the lambda expression with two inputs, on the
other hand, the ManipulateListener defined here doesn't actually use the input.
What's the point of using such bi-function?
(Plus, why use capital "String" as input name? It also confuses with the
class name.)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
