[GitHub] [helix] junkaixue commented on a diff in pull request #2357: Enable MSDS auto refresh for DedicatedZkClient

["junkaixue (via GitHub)" <gi...@apache.org>]

junkaixue commented on code in PR #2357:
URL: https://github.com/apache/helix/pull/2357#discussion_r1091047750


##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java:
##########
@@ -582,4 +588,67 @@ private void checkIfPathContainsShardingKey(String path) {
           + " does not have a valid sharding key or its ZK sharding key is not found in the cached routing data!");
     }
   }
+
+  private String getZkRealm(String path) {
+    if (_routingDataUpdateOnCacheMissEnabled) {
+      try {
+        return updateRoutingDataOnCacheMiss(path);
+      } catch (InvalidRoutingDataException e) {
+        LOG.error(
+            "DedicatedZkClient::getZkRealm: Failed to update routing data due to invalid routing "
+                + "data!", e);
+        throw new MultiZkException(e);
+      }
+    }
+    return _metadataStoreRoutingData.getMetadataStoreRealm(path);
+  }
+
+  /**
+   * Perform a 2-tier routing data cache update:
+   * 1. Do an in-memory update from the singleton RoutingDataManager
+   * 2. Do an I/O based read from the routing data source by resetting RoutingDataManager
+   * @param path
+   * @return
+   * @throws InvalidRoutingDataException
+   */
+  private String updateRoutingDataOnCacheMiss(String path) throws InvalidRoutingDataException {
+    String zkRealm;
+    try {
+      zkRealm = _metadataStoreRoutingData.getMetadataStoreRealm(path);
+    } catch (NoSuchElementException e1) {
+      synchronized (this) {
+        try {
+          zkRealm = _metadataStoreRoutingData.getMetadataStoreRealm(path);
+        } catch (NoSuchElementException e2) {
+          // Try 1) Refresh MetadataStoreRoutingData from RoutingDataManager
+          // This is an in-memory refresh from the Singleton RoutingDataManager - other
+          // ZkClient objects may have triggered a cache refresh, so we first update the
+          // in-memory reference. This refresh only affects this object/thread, so we synchronize
+          // on "this".
+          _metadataStoreRoutingData =
+              RealmAwareZkClient.getMetadataStoreRoutingData(_connectionConfig);
+          try {
+            zkRealm = _metadataStoreRoutingData.getMetadataStoreRealm(path);
+          } catch (NoSuchElementException e3) {
+            synchronized (this) {

Review Comment:
   Reentrant locking usually applies to the logic spread around different code paths.
   
   In the same piece of code why we need to reacquire the lock again?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java:
##########
@@ -183,6 +195,23 @@ public long getLastResetTimestamp() {
     return _lastResetTimestamp;
   }
 
+  /**
+   * Resolves the routing data update interval value from System Properties.
+   */
+  private static long getRoutingDataUpdateInterval() {
+    long routingDataUpdateInterval;
+    try {
+      routingDataUpdateInterval =
+          Long.parseLong(System.getProperty(RoutingSystemPropertyKeys.ROUTING_DATA_UPDATE_INTERVAL_MS));
+      if (routingDataUpdateInterval < 0) {
+        routingDataUpdateInterval = RoutingDataConstants.DEFAULT_ROUTING_DATA_UPDATE_INTERVAL_MS;
+      }
+    } catch (NumberFormatException e) {
+      routingDataUpdateInterval = RoutingDataConstants.DEFAULT_ROUTING_DATA_UPDATE_INTERVAL_MS;

Review Comment:
   Logs?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java:
##########
@@ -582,4 +588,67 @@ private void checkIfPathContainsShardingKey(String path) {
           + " does not have a valid sharding key or its ZK sharding key is not found in the cached routing data!");
     }
   }
+
+  private String getZkRealm(String path) {
+    if (_routingDataUpdateOnCacheMissEnabled) {
+      try {
+        return updateRoutingDataOnCacheMiss(path);
+      } catch (InvalidRoutingDataException e) {
+        LOG.error(
+            "DedicatedZkClient::getZkRealm: Failed to update routing data due to invalid routing "
+                + "data!", e);
+        throw new MultiZkException(e);
+      }
+    }
+    return _metadataStoreRoutingData.getMetadataStoreRealm(path);
+  }
+
+  /**
+   * Perform a 2-tier routing data cache update:
+   * 1. Do an in-memory update from the singleton RoutingDataManager
+   * 2. Do an I/O based read from the routing data source by resetting RoutingDataManager
+   * @param path
+   * @return
+   * @throws InvalidRoutingDataException
+   */
+  private String updateRoutingDataOnCacheMiss(String path) throws InvalidRoutingDataException {
+    String zkRealm;
+    try {
+      zkRealm = _metadataStoreRoutingData.getMetadataStoreRealm(path);

Review Comment:
   Do we need to do it three times? I know this could be similar to safety check logic. But why not just synchronize this on the method and check twice?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java:
##########
@@ -175,6 +178,15 @@ public synchronized void reset() {
     _lastResetTimestamp = System.currentTimeMillis();
   }
 
+  public synchronized void reset(boolean enforceThrottle) {

Review Comment:
   Do you mean forceReset? enforceThrottle is hard to understand.



-- 
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: reviews-unsubscribe@helix.apache.org

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