jiajunwang commented on a change in pull request #1035:
URL: https://github.com/apache/helix/pull/1035#discussion_r432212561
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/manager/ClusterSpectatorManager.java
##########
@@ -0,0 +1,82 @@
+package org.apache.helix.integration.manager;
+
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+
+import org.apache.helix.InstanceType;
+import org.apache.helix.manager.zk.CallbackHandler;
+import org.apache.helix.manager.zk.ZKHelixManager;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ClusterSpectatorManager extends ZKHelixManager implements
Runnable, ZkTestManager {
Review comment:
I think merging spectator and controller managers is simple work. Better
than the current change.
And if you prefer to separate the work, then I would prefer to finish that
change first. Otherwise, this class is purely thrown away work.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -752,6 +756,10 @@ public void handleChildChange(String parentPath,
List<String> currentChilds) {
changeContext.setType(NotificationContext.Type.CALLBACK);
changeContext.setPathChanged(parentPath);
changeContext.setChangeType(_changeType);
+ if (!isReady()) {
Review comment:
This should be the first line in the else block right?
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -539,7 +539,11 @@ private void subscribeChildChange(String path,
NotificationContext.Type callback
logger.debug(_manager.getInstanceName() + " subscribes child-change.
path: " + path
+ ", listener: " + _listener);
}
- _zkClient.subscribeChildChanges(path, this);
+ if (callbackType == Type.INIT) {
Review comment:
Why we need to handle INIT differently?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1865,17 +1934,24 @@ public Object call() throws Exception {
* exist.
*/
public List<String> watchForChilds(final String path) {
+ return watchForChilds(path, false);
+ }
+
+ private List<String> watchForChilds(final String path, boolean
skipWatchingNodeNotExist) {
if (_zookeeperEventThread != null && Thread.currentThread() ==
_zookeeperEventThread) {
throw new IllegalArgumentException("Must not be done in the zookeeper
event thread.");
}
return retryUntilConnected(new Callable<List<String>>() {
@Override
public List<String> call() throws Exception {
- exists(path, true);
+ if (!skipWatchingNodeNotExist) {
Review comment:
If skipWatchingNodeNotExist, then we don't watch the root path? This
change the behavior, right?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1341,8 +1397,11 @@ private void fireChildChangedEvents(final String path,
Set<IZkChildListener> chi
@Override
public void run() throws Exception {
if (!pathStatRecord.pathChecked()) {
- pathStatRecord.recordPathStat(getStat(path, hasListeners(path)
&& pathExists),
- OptionalLong.empty());
+ if (!pathExists) {
+ pathStatRecord.recordPathStat(getStat(path, hasListeners(path)
&& pathExists), OptionalLong.empty());
Review comment:
Same here.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1341,8 +1397,11 @@ private void fireChildChangedEvents(final String path,
Set<IZkChildListener> chi
@Override
public void run() throws Exception {
if (!pathStatRecord.pathChecked()) {
- pathStatRecord.recordPathStat(getStat(path, hasListeners(path)
&& pathExists),
- OptionalLong.empty());
+ if (!pathExists) {
+ pathStatRecord.recordPathStat(getStat(path, hasListeners(path)
&& pathExists), OptionalLong.empty());
+ } else {
+ pathStatRecord.recordPathStat(getStat(path, true, true),
OptionalLong.empty());
Review comment:
If no listener, still need to watch? Why?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##########
@@ -69,12 +69,44 @@
int DEFAULT_SESSION_TIMEOUT = 30 * 1000;
// listener subscription
+ /**
+ * Subscribe the path and the listener will handle child events of the path.
+ * Add exists watch to path if the path does not exist in ZooKeeper server.
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
List<String> subscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle child events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
+ */
+ List<String> subscribeChildChanges(String path, IZkChildListener listener,
boolean skipWatchingNodeNotExist);
+
void unsubscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle data events of the path
+ * Add the exists watch to Zookeeper server even if the path does not exists
in zookeeper server
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
void subscribeDataChanges(String path, IZkDataListener listener);
+ /**
+ * Subscribe the path and the listener will handle data events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
+ */
+ void subscribeDataChanges(String path, IZkDataListener listener, boolean
skipWatchingNodeNotExist);
Review comment:
I feel this API need to be designed carefully.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##########
@@ -69,12 +69,44 @@
int DEFAULT_SESSION_TIMEOUT = 30 * 1000;
// listener subscription
+ /**
+ * Subscribe the path and the listener will handle child events of the path.
+ * Add exists watch to path if the path does not exist in ZooKeeper server.
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
List<String> subscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle child events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
+ */
+ List<String> subscribeChildChanges(String path, IZkChildListener listener,
boolean skipWatchingNodeNotExist);
Review comment:
The old methods can be deprecated since the new one completely covers
the older ones.
Moreover, for this interface, please specify the return value, since it
might be confusing.
1. if null, then the subscribe was skipped.
2. if empty list, then the path has no children node.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##########
@@ -69,12 +69,44 @@
int DEFAULT_SESSION_TIMEOUT = 30 * 1000;
// listener subscription
+ /**
+ * Subscribe the path and the listener will handle child events of the path.
+ * Add exists watch to path if the path does not exist in ZooKeeper server.
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
List<String> subscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle child events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
+ */
+ List<String> subscribeChildChanges(String path, IZkChildListener listener,
boolean skipWatchingNodeNotExist);
+
void unsubscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle data events of the path
+ * Add the exists watch to Zookeeper server even if the path does not exists
in zookeeper server
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
void subscribeDataChanges(String path, IZkDataListener listener);
+ /**
+ * Subscribe the path and the listener will handle data events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
+ */
+ void subscribeDataChanges(String path, IZkDataListener listener, boolean
skipWatchingNodeNotExist);
Review comment:
How does the caller know if the watcher has been done or not?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1054,15 +1078,40 @@ public Stat getStat(final String path) {
}
private Stat getStat(final String path, final boolean watch) {
+ return getStat(path, watch, false);
+ }
+
+ /*
+ * Install watch if there is such node and return the stat
+ *
+ * If useGetData false, use exist(). @param watch would determine if adding
watch
+ * to ZooKeeper server or not.
+ * Note, if @param path does not exist in ZooKeeper server, watch would
still be installed
+ * if @param watch is true.
+ *
+ * If useGetData true, use getData() to add watch. ignore @param watch in
this case.
+ * Note, if @param path does not exist in ZooKeeper server, no watch would
be added.
+ */
+ private Stat getStat(final String path, final boolean watch, final boolean
useGetData) {
long startT = System.currentTimeMillis();
+ Stat stat = null;
try {
- Stat stat = retryUntilConnected(
- () -> ((ZkConnection) getConnection()).getZookeeper().exists(path,
watch));
+ if (!useGetData) {
+ stat = retryUntilConnected(
+ () -> ((ZkConnection) getConnection()).getZookeeper().exists(path,
watch));
+ } else {
+ stat = new Stat();
+ Stat finalStat = stat;
+ retryUntilConnected(
+ () -> ((ZkConnection)
getConnection()).getZookeeper().getData(path, true, finalStat));
+ LOG.debug("getstat() invoked with useGetData() with path: " + path);
+ }
record(path, null, startT, ZkClientMonitor.AccessType.READ);
return stat;
} catch (ZkNoNodeException e) {
+ LOG.debug("getstat() invoked path: " + path + " null" + " useGetData:" +
useGetData);
record(path, null, startT, ZkClientMonitor.AccessType.READ);
- throw e;
+ return null;
Review comment:
It would be easier to understand that we wrap the getData() call with
this try catch.
For the exists() call, it will never throw ZkNoNodeException, right?
In this case, I guess this try catch branch is not necessary here?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1304,7 +1356,11 @@ private void fireDataChangedEvents(final String path,
Set<IZkDataListenerEntry>
public void run() throws Exception {
if (!pathStatRecord.pathChecked()) {
// getStat will re-install watcher only when the path exists
- pathStatRecord.recordPathStat(getStat(path, pathExists),
notificationTime);
+ if (!pathExists) {
Review comment:
Put the getStat call inside the recordPathStat parameter list is no
longer clean.
Please create a local var for the state and pass it to the record method.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##########
@@ -69,12 +69,44 @@
int DEFAULT_SESSION_TIMEOUT = 30 * 1000;
// listener subscription
+ /**
+ * Subscribe the path and the listener will handle child events of the path.
+ * Add exists watch to path if the path does not exist in ZooKeeper server.
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ */
List<String> subscribeChildChanges(String path, IZkChildListener listener);
+ /**
+ * Subscribe the path and the listener will handle child events of the path
+ * WARNING: if the path is created after deletion, users need to
re-subscribe the path
+ * @param path The zookeeper path
+ * @param listener Instance of {@link IZkDataListener}
+ * @param skipWatchingNodeNotExist Ture means not installing any watch if
path does not exist.
Review comment:
I feel this boolean is not straightforward to understand. Shall we add
an opposite boolean, watchNonExistPath? That is easier to understand, IMO.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1304,7 +1356,11 @@ private void fireDataChangedEvents(final String path,
Set<IZkDataListenerEntry>
public void run() throws Exception {
if (!pathStatRecord.pathChecked()) {
// getStat will re-install watcher only when the path exists
- pathStatRecord.recordPathStat(getStat(path, pathExists),
notificationTime);
+ if (!pathExists) {
+ pathStatRecord.recordPathStat(getStat(path, false),
notificationTime);
+ } else {
+ pathStatRecord.recordPathStat(getStat(path, true, true),
notificationTime);
Review comment:
Same here, if path does not exists, the old logic is skip watching,
right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
