[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #2802: Retry opening the TSocket in KyuubiSyncThriftClient

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

ulysses-you commented on code in PR #2802:
URL: https://github.com/apache/incubator-kyuubi/pull/2802#discussion_r887442386


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala:
##########
@@ -106,23 +106,13 @@ class KyuubiSyncThriftClient private (
   }
 
   private def withRetryingRequest[T](block: => T, request: String): T = withLockAcquired {
-    var attemptCount = 1
-
-    var resp: T = null.asInstanceOf[T]
-    while (attemptCount <= maxAttempts && resp == null) {
-      try {
-        resp = block
-        remoteEngineBroken = false
-      } catch {
-        case e: TException if attemptCount < maxAttempts && isConnectionValid() =>
-          warn(s"Failed to execute $request after $attemptCount/$maxAttempts times, retrying", e)
-          attemptCount += 1
-          Thread.sleep(100)
-        case e: Throwable =>
-          error(s"Failed to execute $request after $attemptCount/$maxAttempts times, aborting", e)
-          throw e
-      }
-    }
+    val (resp, remoteEngineBrokenRetValue) = KyuubiSyncThriftClient.withRetryingRequestNoLock(
+      block,
+      request,
+      maxAttempts,
+      remoteEngineBroken,
+      isConnectionValid)
+    remoteEngineBroken = remoteEngineBrokenRetValue;

Review Comment:
   The semantics seems a little different with previous. Before we only reset `remoteEngineBroken` if we succeeded to do request, and if failed the `remoteEngineBroken` depend on the probe so we do not change it.
   
   So, we do not pass `remoteEngineBroken` to the new method `withRetryingRequestNoLock`, and change the semantics of the returned value `remoteEngineBrokenRetValue` to `shouldResetRemoteEngineBroken`, then if return true, we change  `remoteEngineBroken` to false, else we do nothing which is same with previous.



-- 
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: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org