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