kaisun2000 commented on a change in pull request #1119:
URL: https://github.com/apache/helix/pull/1119#discussion_r459123015
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1221,21 +1234,84 @@ private void reconnect() {
}
}
+
+
+ private void doAsyncSync(final ZooKeeper zk, final String path, final long
startT,
+ final ZkAsyncCallbacks.SyncCallbackHandler cb) {
+ zk.sync(path, cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor,
startT, 0, true) {
+ @Override
+ protected void doRetry() throws Exception {
+ doAsyncSync(zk, path, System.currentTimeMillis(), cb);
+ }
+ });
+ }
+
+ /*
+ * Note, issueSync takes a ZooKeeper (client) object and pass it to
doAsyncSync().
+ * The reason we do this is that we want to ensure each new session event
is preceded with exactly
+ * one sync() to server. The sync() is to make sure the server would not
see stale data.
+ *
+ * ZooKeeper client object has an invariant of each object has one session.
With this invariant
+ * we can achieve each one sync() to server upon new session establishment.
The reasoning is:
+ * issueSync() is called when fireNewSessionEvents() which in under
eventLock of ZkClient. Thus
+ * we are guaranteed the ZooKeeper object passed in would have the new
incoming sessionId. If by
+ * the time sync() is invoked, the session expires. The sync() would fail
with a stale session.
+ * This is exactly what we want. The newer session would ensure another
fireNewSessionEvents.
+ */
+ private boolean issueSync(String sessionId, ZooKeeper zk) throws
ZkInterruptedException {
Review comment:
See my reply (just now) of the after fact checking cons here:
> Just saw this comment.
The gist is that the checking of sessionId and doAsyncSync() is not atomic.
After if (zk.getSessionId().equals(sessionId)) succeed, the Zookeeper object
may be changed due to a new session established. This would make the current
doAsyncSync() work for the new session. Since the new session would trigger
another fireNewSession(), which would put another retrySync(). Then we will
have another sync() to the Zookeeper for the new session. So in sum, the new
session can be two sync() in a row.
Back to your concern about old Zk object throw exceptions, here is how I
look at it:
First, async call (sync() in this case), would not throw exception. The
error code would from the callback (invoked in eventthread of ZooKeeper) would
tell what is wrong. The checking is in `protected boolean needRetry(int rc)`.
----------------------------------------------------------------
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]
