You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "junkaixue (via GitHub)" <gi...@apache.org> on 2023/04/28 15:42:46 UTC
[GitHub] [helix] junkaixue commented on a diff in pull request #2460: Add find and remove for recursive persist listener trie
junkaixue commented on code in PR #2460:
URL: https://github.com/apache/helix/pull/2460#discussion_r1180550227
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/util/ZkPathRecursiveWatcherTrie.java:
##########
@@ -139,17 +139,88 @@ public void addRecursiveListener(final String path, RecursivePersistListener lis
if (path.isEmpty()) {
throw new IllegalArgumentException("Empty path: " + path);
}
- final List<String> pathComponents = split(path);
+ final List<String> pathComponents = split(path);
- synchronized(this) {
+ synchronized (this) {
TrieNode parent = _rootNode;
for (final String part : pathComponents) {
- parent = parent.getChildren().computeIfAbsent(part, (p)-> new TrieNode(part) );
+ parent = parent.getChildren().computeIfAbsent(part, (p) -> new TrieNode(part));
}
parent._recursiveListeners.add(listener);
}
}
+ public Set<RecursivePersistListener> getAllRecursiveListeners(String path) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ final List<String> pathComponents = split(path);
+ Set<RecursivePersistListener> result = new HashSet<>();
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ for (final String element : pathComponents) {
+ cur = cur.getChild(element);
+ if (cur == null) {
+ break;
+ }
+ result.addAll(cur.getRecursiveListeners());
+ }
Review Comment:
This piece of code can be generalized:
synchronized (this) {
TrieNode cur = _rootNode;
for (final String element : pathComponents) {
cur = cur.getChild(element);
if (cur == null) {
break;
}
callable.call(cur);
}
For this call() {
result.addAll(cur.getRecursiveListeners());
}
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/util/ZkPathRecursiveWatcherTrie.java:
##########
@@ -139,17 +139,88 @@ public void addRecursiveListener(final String path, RecursivePersistListener lis
if (path.isEmpty()) {
throw new IllegalArgumentException("Empty path: " + path);
}
- final List<String> pathComponents = split(path);
+ final List<String> pathComponents = split(path);
- synchronized(this) {
+ synchronized (this) {
TrieNode parent = _rootNode;
for (final String part : pathComponents) {
- parent = parent.getChildren().computeIfAbsent(part, (p)-> new TrieNode(part) );
+ parent = parent.getChildren().computeIfAbsent(part, (p) -> new TrieNode(part));
}
parent._recursiveListeners.add(listener);
}
}
+ public Set<RecursivePersistListener> getAllRecursiveListeners(String path) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ final List<String> pathComponents = split(path);
+ Set<RecursivePersistListener> result = new HashSet<>();
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ for (final String element : pathComponents) {
+ cur = cur.getChild(element);
+ if (cur == null) {
+ break;
+ }
+ result.addAll(cur.getRecursiveListeners());
+ }
+ }
+
+ return result;
+ }
+
+ /**
+ * Removing a RecursivePersistWatcherListener on a path.
+ *
+ * Delete a path from the nearest trie node to current node if this is the only listener and there
+ * is no child on the trie node.
+ *
+ * @param path the of the lister registered
+ * @param listener the RecursivePersistListener to be removed
+ */
+ public void removeRecursiveListener(final String path, RecursivePersistListener listener) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ if (path.length() == 0) {
+ throw new IllegalArgumentException("Invalid path: " + path);
+ }
+ final List<String> pathComponents = split(path);
+
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ TrieNode highestNodeForDelete =
+ null; // track the highest node that from that node to leaf node.
+ TrieNode prevDeletable = _rootNode;
+ for (final String part : pathComponents) {
+ cur = cur.getChild(part);
+ if (cur == null) {
+ return;
+ }
+
+ boolean candidateToDelete =
+ (cur.getChildren().size() == 1 && cur.getRecursiveListeners().size() == 0) || (
+ cur.getChildren().size() == 0 && cur.getRecursiveListeners().size() == 1 && cur
+ .getRecursiveListeners().contains(listener));
+ if (candidateToDelete) {
+ if (highestNodeForDelete == null) {
+ highestNodeForDelete = cur;
+ }
+ } else {
+ prevDeletable = cur;
+ highestNodeForDelete = null;
+ }
+ }
+ if (!cur.getRecursiveListeners().contains(listener)) {
+ return;
+ }
+ cur.getRecursiveListeners().remove(listener);
+
+ if (highestNodeForDelete != null) {
+ prevDeletable.getChildren().remove(highestNodeForDelete.getValue());
+ }
Review Comment:
This will be generalized as the call() {
xxx
}
if (!cur.getRecursiveListeners().contains(listener)) {
return;
}
For this line, you have have a flag in original code, if flag == false, then stop search.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/util/ZkPathRecursiveWatcherTrie.java:
##########
@@ -139,17 +139,88 @@ public void addRecursiveListener(final String path, RecursivePersistListener lis
if (path.isEmpty()) {
throw new IllegalArgumentException("Empty path: " + path);
}
- final List<String> pathComponents = split(path);
+ final List<String> pathComponents = split(path);
- synchronized(this) {
+ synchronized (this) {
TrieNode parent = _rootNode;
for (final String part : pathComponents) {
- parent = parent.getChildren().computeIfAbsent(part, (p)-> new TrieNode(part) );
+ parent = parent.getChildren().computeIfAbsent(part, (p) -> new TrieNode(part));
}
parent._recursiveListeners.add(listener);
}
}
+ public Set<RecursivePersistListener> getAllRecursiveListeners(String path) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ final List<String> pathComponents = split(path);
+ Set<RecursivePersistListener> result = new HashSet<>();
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ for (final String element : pathComponents) {
+ cur = cur.getChild(element);
+ if (cur == null) {
+ break;
+ }
+ result.addAll(cur.getRecursiveListeners());
+ }
+ }
+
+ return result;
+ }
+
+ /**
+ * Removing a RecursivePersistWatcherListener on a path.
+ *
+ * Delete a path from the nearest trie node to current node if this is the only listener and there
+ * is no child on the trie node.
+ *
+ * @param path the of the lister registered
+ * @param listener the RecursivePersistListener to be removed
+ */
+ public void removeRecursiveListener(final String path, RecursivePersistListener listener) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ if (path.length() == 0) {
+ throw new IllegalArgumentException("Invalid path: " + path);
+ }
+ final List<String> pathComponents = split(path);
+
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ TrieNode highestNodeForDelete =
+ null; // track the highest node that from that node to leaf node.
+ TrieNode prevDeletable = _rootNode;
+ for (final String part : pathComponents) {
+ cur = cur.getChild(part);
+ if (cur == null) {
+ return;
+ }
+
+ boolean candidateToDelete =
Review Comment:
Better add some explanation here. For the people does not have context may get confused.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/util/ZkPathRecursiveWatcherTrie.java:
##########
@@ -139,17 +139,88 @@ public void addRecursiveListener(final String path, RecursivePersistListener lis
if (path.isEmpty()) {
throw new IllegalArgumentException("Empty path: " + path);
}
- final List<String> pathComponents = split(path);
+ final List<String> pathComponents = split(path);
- synchronized(this) {
+ synchronized (this) {
TrieNode parent = _rootNode;
for (final String part : pathComponents) {
- parent = parent.getChildren().computeIfAbsent(part, (p)-> new TrieNode(part) );
+ parent = parent.getChildren().computeIfAbsent(part, (p) -> new TrieNode(part));
}
parent._recursiveListeners.add(listener);
}
}
+ public Set<RecursivePersistListener> getAllRecursiveListeners(String path) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ final List<String> pathComponents = split(path);
+ Set<RecursivePersistListener> result = new HashSet<>();
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ for (final String element : pathComponents) {
+ cur = cur.getChild(element);
+ if (cur == null) {
+ break;
+ }
+ result.addAll(cur.getRecursiveListeners());
+ }
+ }
+
+ return result;
+ }
+
+ /**
+ * Removing a RecursivePersistWatcherListener on a path.
+ *
+ * Delete a path from the nearest trie node to current node if this is the only listener and there
+ * is no child on the trie node.
+ *
+ * @param path the of the lister registered
+ * @param listener the RecursivePersistListener to be removed
+ */
+ public void removeRecursiveListener(final String path, RecursivePersistListener listener) {
+ Objects.requireNonNull(path, "Path cannot be null");
+
+ if (path.length() == 0) {
+ throw new IllegalArgumentException("Invalid path: " + path);
+ }
+ final List<String> pathComponents = split(path);
+
+ synchronized (this) {
+ TrieNode cur = _rootNode;
+ TrieNode highestNodeForDelete =
+ null; // track the highest node that from that node to leaf node.
+ TrieNode prevDeletable = _rootNode;
+ for (final String part : pathComponents) {
+ cur = cur.getChild(part);
+ if (cur == null) {
+ return;
+ }
+
+ boolean candidateToDelete =
+ (cur.getChildren().size() == 1 && cur.getRecursiveListeners().size() == 0) || (
Review Comment:
This condition means this node has only one subpath. If subpath contains listener and removed, then why not remove this node itself. This node should be empty as well.
--
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