[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #38: CASSANDRASC-44 Refactor health check to use vertx timer

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

yifan-c commented on code in PR #38:
URL: https://github.com/apache/cassandra-sidecar/pull/38#discussion_r992592120


##########
src/main/java/org/apache/cassandra/sidecar/CassandraSidecarDaemon.java:
##########
@@ -97,6 +101,20 @@ private void validate()
 
     }
 
+    /**
+     * Checks the health of every instance configured in the {@link Configuration#getInstancesConfig()}.
+     * The health check is executed in a blocking thread to prevent the event-loop threads from blocking.
+     *
+     * @param timerId the ID of the periodic timer
+     */
+    private void healthCheck(Long timerId)
+    {
+        config.getInstancesConfig()
+              .instances()
+              .forEach(instanceMetadata ->
+                       vertx.executeBlocking(promise -> instanceMetadata.delegate().healthCheck()));

Review Comment:
   If my understanding is correct. 
   In this setup, if the prior check takes a longer time that is greater than the interval, there could be 2 threads that runs `healthCheck` concurrently. Due to the check is `synchronized`, there will be one thread being blocked. 
   In the prior setup, no additional thread is to be blocked. 
   
   I do not mean that the prior one is better.
   
   One step back, the health check is something we want to perform every X seconds. If a prolonged check just finished, the next one should not be immediately performed, which is the problem for both setups. 
   
   Can we add a `shouldCheck(timeInMillis)` method and skip a check if it has just checked very recently? 



##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -127,7 +95,7 @@ public synchronized void checkSession()
      * Should be called on initial connect as well as when a server comes back since it might be from an upgrade
      * synchronized so we don't flood the DB with version requests
      *
-     * If the healthcheck determines we've changed versions, it should load the proper adapter
+     * <p>If the healthcheck determines we've changed versions, it should load the proper adapter

Review Comment:
   nit: either remove the `<p>` tag or close it with `</p>`. I do not see the advantage of adding the html tag, so lean to removing. 



-- 
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: pr-unsubscribe@cassandra.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org