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

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

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


##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java:
##########
@@ -183,6 +199,25 @@ public long getLastResetTimestamp() {
     return _lastResetTimestamp;
   }
 
+  /**
+   * Resolves the routing data update interval value from System Properties.
+   */
+  public void parseRoutingDataUpdateInterval() {

Review Comment:
   Does this have to be public? I thought this is only used during construction?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java:
##########
@@ -582,4 +588,63 @@ 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);

Review Comment:
   nit: I think usually we put additional message to the exception before re-throw. We probably don't need the error log that way.



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java:
##########
@@ -582,4 +588,63 @@ 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);

Review Comment:
   For my own understanding, what and how is the difference between the first and second check? 



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/routing/RoutingDataManager.java:
##########
@@ -59,6 +61,9 @@ public class RoutingDataManager {
   // Tracks the time at which reset() was called last. Used to throttle reset()
   private volatile long _lastResetTimestamp;
 
+  // Interval value used to throttle reset()
+  private long _routingDataUpdateInterval;
+
   // Singleton instance
   private static RoutingDataManager _instance;

Review Comment:
   not directly related to your PR, but this should have been a volatile in the first place.



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