omalley commented on code in PR #5298: URL: https://github.com/apache/hadoop/pull/5298#discussion_r1083100964
########## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java: ########## @@ -1730,4 +1750,39 @@ private static boolean isReadCall(Method method) { } return !method.getAnnotationsByType(ReadOnly.class)[0].activeOnly(); } + + /** + * Checks and sets last refresh time for a namespace's stateId. + * Returns true if refresh time is newer than threshold. + * Otherwise, return false and call should be handled by active namenode. + * @param nsId namespaceID + */ + @VisibleForTesting + boolean isNamespaceStateIdFresh(String nsId) { + if (activeNNStateIdRefreshPeriodMs < 0) { + LOG.debug("Skipping freshness check and returning True since" Review Comment: Move this to the initialization, so that we only get the log once rather than every call. ########## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RBFConfigKeys.java: ########## @@ -201,6 +201,10 @@ public class RBFConfigKeys extends CommonConfigurationKeysPublic { FEDERATION_ROUTER_PREFIX + "observer.federated.state.propagation.maxsize"; public static final int DFS_ROUTER_OBSERVER_FEDERATED_STATE_PROPAGATION_MAXSIZE_DEFAULT = 5; + public static final String DFS_ROUTER_OBSERVER_STATE_ID_REFRESH_PERIOD_KEY = + FEDERATION_ROUTER_PREFIX + "observer.state.id.refresh.period"; + public static final String DFS_ROUTER_OBSERVER_STATE_ID_REFRESH_PERIOD_DEFAULT = "5s"; Review Comment: Let's make this longer, like 10 or 15s? ########## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java: ########## @@ -211,13 +222,19 @@ public RouterRpcClient(Configuration conf, Router router, this.observerReadEnabledDefault = conf.getBoolean( RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_DEFAULT_KEY, RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_DEFAULT_VALUE); - String[] observerReadOverrides = conf.getStrings(RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_OVERRIDES); + String[] observerReadOverrides = + conf.getStrings(RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_OVERRIDES); if (observerReadOverrides != null) { observerReadEnabledOverrides.addAll(Arrays.asList(observerReadOverrides)); } if (this.observerReadEnabledDefault) { LOG.info("Observer read is enabled for router."); } + this.activeNNStateIdRefreshPeriodMs = conf.getTimeDuration( + RBFConfigKeys.DFS_ROUTER_OBSERVER_STATE_ID_REFRESH_PERIOD_KEY, + RBFConfigKeys.DFS_ROUTER_OBSERVER_STATE_ID_REFRESH_PERIOD_DEFAULT, + TimeUnit.SECONDS, TimeUnit.MILLISECONDS); + this.lastActiveNNRefreshTimes = new HashMap<>(); Review Comment: Don't you need locking around the access to this hashmap? ########## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java: ########## @@ -1730,4 +1750,39 @@ private static boolean isReadCall(Method method) { } return !method.getAnnotationsByType(ReadOnly.class)[0].activeOnly(); } + + /** + * Checks and sets last refresh time for a namespace's stateId. + * Returns true if refresh time is newer than threshold. + * Otherwise, return false and call should be handled by active namenode. + * @param nsId namespaceID + */ + @VisibleForTesting + boolean isNamespaceStateIdFresh(String nsId) { + if (activeNNStateIdRefreshPeriodMs < 0) { + LOG.debug("Skipping freshness check and returning True since" + + RBFConfigKeys.DFS_ROUTER_OBSERVER_STATE_ID_REFRESH_PERIOD_KEY + + " is less than 0"); + return true; + } + + Call call = Server.getCurCall().get(); Review Comment: Let's just use the current time here. ########## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java: ########## @@ -1730,4 +1750,39 @@ private static boolean isReadCall(Method method) { } return !method.getAnnotationsByType(ReadOnly.class)[0].activeOnly(); } + + /** + * Checks and sets last refresh time for a namespace's stateId. + * Returns true if refresh time is newer than threshold. + * Otherwise, return false and call should be handled by active namenode. + * @param nsId namespaceID + */ + @VisibleForTesting + boolean isNamespaceStateIdFresh(String nsId) { + if (activeNNStateIdRefreshPeriodMs < 0) { + LOG.debug("Skipping freshness check and returning True since" Review Comment: Actually, you should also use expansion pattern rather than string concatenation. LOG.debug("Skipping ... {} is less than zero.", RBFConfigKeys....); -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org