[GitHub] [helix] jiajunwang commented on a change in pull request #1592: Cancel async callback when exception is thrown

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1592:
URL: https://github.com/apache/helix/pull/1592#discussion_r540585975



##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncRetryThread.java
##########
@@ -49,7 +49,9 @@ public void run() {
           context.cancel();
           interrupt();
         } catch (Throwable e) {
-          LOG.error("Error retrying callback " + context, e);
+          LOG.error("Error retrying callback {}, cancelling it", context, e);
+          // Cancel the context so the upstream caller can stop waiting
+          context.cancel();

Review comment:
       No, if retry was done successfully, then cancel it prematurely will block the normal logic.

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1927,15 +1927,23 @@ private void doAsyncCreate(final String path, final byte[] data, final CreateMod
       final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb,
       final String expectedSessionId) {
     retryUntilConnected(() -> {
-      getExpectedZookeeper(expectedSessionId)
-          .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode, cb,
-              new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false) {
-                @Override
-                protected void doRetry() {
-                  doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
-                      expectedSessionId);
-                }
-              });
+      final ZooKeeper zk;
+      try {
+        zk = getExpectedZookeeper(expectedSessionId);
+      } catch (ZkSessionMismatchedException e) {
+        // Use BADARGUMENTS rc to represent session mismatch error (invalid expectedSession)
+        // and not retry.
+        cb.processResult(KeeperException.Code.BADARGUMENTS.intValue(), path,
+            new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+        throw e;
+      }
+      zk.create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode, cb,
+          new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false) {
+            @Override
+            protected void doRetry() {
+              doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb, expectedSessionId);

Review comment:
       Why we need these given we have already cancel it in the doRetry()?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org