[incubator-kyuubi] branch master updated: [KYUUBI #2594] Fix flaky Test - support engine alive probe to fast fail on engine broken

[fe...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

feiwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-kyuubi.git


The following commit(s) were added to refs/heads/master by this push:
     new 56efdf8cc [KYUUBI #2594] Fix flaky Test - support engine alive probe to fast fail on engine broken
56efdf8cc is described below

commit 56efdf8cc13c8a517d2a19b2f1f8f44bed6f1e4f
Author: Fei Wang <fw...@ebay.com>
AuthorDate: Wed May 18 16:57:58 2022 +0800

    [KYUUBI #2594] Fix flaky Test - support engine alive probe to fast fail on engine broken
    
    ### _Why are the changes needed?_
    
    To close #2594
    
    For the liveness probe, it marks the remote engine broken if the last alive time detection is timeout.
    
    ```
                    if (now - engineLastAlive > engineAliveTimeout) {
                      error("Mark the engine not alive with no recent alive probe success:" +
                        s" ${now - engineLastAlive} ms exceeds timeout $engineAliveTimeout ms")
                      remoteEngineBroken = true
                    }
    ```
    But now the engineLastAlive variable is not volatile.
    So we set the engineLastAlive as volatile variable in this pr to prevent flaky test.
    
    And now we are using `scheduleAtFixedRate`, it will trigger the liveness detection in fixed rate and regardless whether the last one has completed.
    
    For scheduleWithFixedDelay,
    ```
    Creates and executes a periodic action that becomes enabled first after the given initial delay, and subsequently with the given delay between the termination of one execution and the commencement of the next. If any execution of the task encounters an exception, subsequent executions are suppressed. Otherwise, the task will only terminate via cancellation or termination of the executor.
    ```
    
    For scheduleAtFixedRate,
    ```
    Creates and executes a periodic action that becomes enabled first after the given initial delay, and subsequently with the given period; that is executions will commence after initialDelay then initialDelay+period, then initialDelay + 2 * period, and so on. If any execution of the task encounters an exception, subsequent executions are suppressed. Otherwise, the task will only terminate via cancellation or termination of the executor. If any execution of this task takes longer than it [...]
    ```
    
    We should use scheduleWithFixedDelay, so that the liveness probe detections are in sequence.
    
    ### _How was this patch tested?_
    - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
    
    - [ ] Add screenshots for manual tests if appropriate
    
    - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
    
    Closes #2685 from turboFei/engine_alive.
    
    Closes #2594
    
    0c969386 [Fei Wang] set engineLastAlive volatile
    960d6080 [Fei Wang] Use scheduleWithFixedDelay instead of scheduleAtFixedRate
    
    Authored-by: Fei Wang <fw...@ebay.com>
    Signed-off-by: Fei Wang <fw...@ebay.com>
---
 .../main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala
index 419a48249..917449dfb 100644
--- a/kyuubi-server/src/main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala
+++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/client/KyuubiSyncThriftClient.scala
@@ -53,7 +53,7 @@ class KyuubiSyncThriftClient private (
   @volatile private var remoteEngineBroken: Boolean = false
   private val engineAliveProbeClient = engineAliveProbeProtocol.map(new TCLIService.Client(_))
   private var engineAliveThreadPool: ScheduledExecutorService = _
-  private var engineLastAlive: Long = _
+  @volatile private var engineLastAlive: Long = _
 
   private def startEngineAliveProbe(): Unit = {
     engineAliveThreadPool = ThreadUtils.newDaemonSingleThreadScheduledExecutor(
@@ -85,7 +85,7 @@ class KyuubiSyncThriftClient private (
       }
     }
     engineLastAlive = System.currentTimeMillis()
-    engineAliveThreadPool.scheduleAtFixedRate(
+    engineAliveThreadPool.scheduleWithFixedDelay(
       task,
       engineAliveProbeInterval,
       engineAliveProbeInterval,