rahulrane50 commented on code in PR #2325:
URL: https://github.com/apache/helix/pull/2325#discussion_r1055017879
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -239,16 +250,7 @@ protected ZkClient(IZkConnection zkConnection, int
connectionTimeout, long opera
} else {
LOG.info("ZkClient monitor key or type is not provided. Skip
monitoring.");
}
-
- connect(connectionTimeout, this);
-
- try {
- if (_monitor != null) {
- _monitor.register();
- }
- } catch (JMException e){
- LOG.error("Error in creating ZkClientMonitor", e);
- }
+ _watcher = watcher == null ? this : watcher;
Review Comment:
neat : We should add comment here. If I understand correctly then either
"Raw ZkClient" passes "watcher" in "native Zkclient"'s instantiation or legacy
code instantiates native zkclient without any watcher. If that's the case then
we should add comments at bot places (native zk client and raw zk client) IMHO.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1639,7 +1641,7 @@ private void fireNewSessionEvents() {
sessionId) {
@Override
public void run() throws Exception {
- if (issueSync(zk) == false) {
+ if (!issueSync(zk)) {
Review Comment:
smart!
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2396,12 +2395,17 @@ private void checkDataSizeLimit(String path, byte[]
data) {
}
public void watchForData(final String path) {
- watchForData(path, false);
+ watchForData(path, false, false);
}
- private boolean watchForData(final String path, boolean
skipWatchingNonExistNode) {
+ public boolean watchForData(final String path, boolean
skipWatchingNonExistNode, boolean persistListener) {
try {
- if (skipWatchingNonExistNode) {
+ if (persistListener) {
+ retryUntilConnected(() -> {
+ ((ZkConnection) getConnection()).getZookeeper().addWatch(path,
AddWatchMode.PERSISTENT);
+ return true;
Review Comment:
neat: If we are anyways going to return "true" here means if execution
enters into "persistListener" branch then it will never enter following
branches right? in that case we can change it to "if" rather than "else if"?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1566,7 +1568,7 @@ private void reconnect() {
getEventLock().lock();
try {
ZkConnection connection = ((ZkConnection) getConnection());
- connection.reconnect(this);
+ connection.reconnect(_watcher == null ? this : _watcher);
Review Comment:
we can directly use "_watcher" object here right?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2396,12 +2395,17 @@ private void checkDataSizeLimit(String path, byte[]
data) {
}
public void watchForData(final String path) {
- watchForData(path, false);
+ watchForData(path, false, false);
}
- private boolean watchForData(final String path, boolean
skipWatchingNonExistNode) {
+ public boolean watchForData(final String path, boolean
skipWatchingNonExistNode, boolean persistListener) {
try {
- if (skipWatchingNonExistNode) {
+ if (persistListener) {
+ retryUntilConnected(() -> {
+ ((ZkConnection) getConnection()).getZookeeper().addWatch(path,
AddWatchMode.PERSISTENT);
Review Comment:
neat: can we please add more comments for this new watch type?
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1424,7 +1426,7 @@ public Boolean call() throws Exception {
} finally {
long endT = System.currentTimeMillis();
if (LOG.isTraceEnabled()) {
- LOG.trace("zkclient exists, path: {}, time: {} ms", _uid, path, (endT
- startT));
+ LOG.trace("zkclient {} exists, path: {}, time: {} ms", _uid, path,
(endT - startT));
Review Comment:
neat. thanks for fixing all logging bugs, I'm really curious do we even have
any mechanism or test code path to verify this tracelog branches in code.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2553,6 +2557,13 @@ public void connect(final long
maxMsToWaitUntilConnected, Watcher watcher)
close();
}
}
+ try {
+ if (_monitor != null) {
+ _monitor.register();
Review Comment:
Umm connect() can be called multiple times right, do we want to register
monitor every time? Previously it used to register only when native zkclient
was instantiated.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2499,7 +2503,7 @@ public Object call() throws Exception {
* @throws IllegalStateException
* if the connection timed out due to thread interruption
*/
- public void connect(final long maxMsToWaitUntilConnected, Watcher watcher)
+ public void connect(final long maxMsToWaitUntilConnected, @Nullable Watcher
watcher)
Review Comment:
curious would there be a case when meta-client or caller explicitly passes
"null" as watcher? (meaning it doesn't want to use native zk client as watcher
but just null)
--
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]
