[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2687: [KYUUBI #2686] Fix lock bug if engine initialization timeout

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

yaooqinn commented on code in PR #2687:
URL: https://github.com/apache/incubator-kyuubi/pull/2687#discussion_r876636462


##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperDiscoveryClient.scala:
##########
@@ -143,9 +143,34 @@ class ZookeeperDiscoveryClient(conf: KyuubiConf) extends DiscoveryClient {
       try {
         lock = new InterProcessSemaphoreMutex(zkClient, lockPath)
         // Acquire a lease. If no leases are available, this method blocks until either the
-        // maximum number of leases is increased or another client/process closes a lease
-        lock.acquire(timeout, unit)
+        // maximum number of leases is increased or another client/process closes a lease.
+        //
+        // Here, we should throw exception if timeout during acquiring lock.
+        // Let's say we have three clients with same request lock to two kyuubi server instances.
+        //
+        //  client A  --->  kyuubi X  -- first acquired  \
+        //  client B  --->  kyuubi X  -- second acquired --  zookeeper
+        //  client C  --->  kyuubi Y  -- third acquired  /
+        //
+        // The first client A acqiured the lock then B and C are blocked until A release the lock,
+        // with the A created engine state:
+        //   - SUCCESS
+        //     B acquired the lock then get engine ref and release the lock.
+        //     C acquired the lock then get engine ref and release the lock.
+        //   - FAILED or TIMEOUT
+        //     B acquired the lock then try to create engine again if not timeout.
+        //     C should be timeout and throw exception back to client. This fast fail
+        //     to avoid client too long to waiting in concurrent.
+
+        // Return false means we are timeout
+        val acquired = lock.acquire(timeout, unit)

Review Comment:
   use TimeUnit.MILLISECONDS directly, the timeout is always millis



-- 
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