[GitHub] [hadoop] omalley commented on a diff in pull request #5298: HDFS-16890: RBF: Ensures router periodically refreshes its record of a namespace's state.

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

omalley commented on code in PR #5298:
URL: https://github.com/apache/hadoop/pull/5298#discussion_r1100744979


##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -1730,4 +1755,30 @@ 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) {
+      return true;
+    }
+
+    long currentTimeMs = Time.monotonicNow();
+    LongAccumulator latestRefreshTimeMs = lastActiveNNRefreshTimes
+        .computeIfAbsent(nsId, key -> new LongAccumulator(Math::max, 0));
+
+    return ((currentTimeMs - latestRefreshTimeMs.get()) <= activeNNStateIdRefreshPeriodMs);
+  }
+
+  private void refreshTimeOfLastCallToActiveNameNode(String namespaceId) {
+    LongAccumulator latestRefreshTimeMs = lastActiveNNRefreshTimes

Review Comment:
   I'd suggest putting this common code into a method like:
   
   LongAccumulator getLastCallToActive(String nsId) { ... }
   
   Then the other lines could mostly be inline:
   
   return currentTimeMs - getLastCallToActive(nsId).get() <= activeNNStateIdRefreshPeriodMs;
   
   and
   
   getLastCallToActive(nsId).accumulate(Time.monotonicNow());



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -1702,10 +1724,12 @@ private List<? extends FederationNamenodeContext> getOrderedNamenodes(String nsI
       boolean isObserverRead) throws IOException {
     final List<? extends FederationNamenodeContext> namenodes;
 
-    if (RouterStateIdContext.getClientStateIdFromCurrentCall(nsId) > Long.MIN_VALUE) {
+    if (isNamespaceStateIdFresh(nsId)
+        && (RouterStateIdContext.getClientStateIdFromCurrentCall(nsId) > Long.MIN_VALUE)) {
       namenodes = namenodeResolver.getNamenodesForNameserviceId(nsId, isObserverRead);
     } else {
       namenodes = namenodeResolver.getNamenodesForNameserviceId(nsId, false);
+      refreshTimeOfLastCallToActiveNameNode(nsId);

Review Comment:
   Shouldn't this be updated on whether we went to the active? In other words, if isObserverRead is false, this should be updated also.



-- 
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-unsubscribe@hadoop.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org