You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "qqu0127 (via GitHub)" <gi...@apache.org> on 2023/04/17 19:53:46 UTC
[GitHub] [helix] qqu0127 commented on a diff in pull request #2432: Use persist watcher for listener registration in ZkClient (when configured)
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org