[GitHub] [hadoop] simbadzina commented on a change in pull request #4092: HADOOP-18167. Add metrics to track delegation token secret manager op…

[GitBox <gi...@apache.org>]

simbadzina commented on a change in pull request #4092:
URL: https://github.com/apache/hadoop/pull/4092#discussion_r833755420



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -429,11 +445,16 @@ private synchronized void removeExpiredKeys() {
     byte[] password = createPassword(identifier.getBytes(), currentKey.getKey());
     DelegationTokenInformation tokenInfo = new DelegationTokenInformation(now
         + tokenRenewInterval, password, getTrackingIdIfEnabled(identifier));
+    long start = Time.monotonicNow();
     try {
       storeToken(identifier, tokenInfo);
     } catch (IOException ioe) {
       LOG.error("Could not store token " + formatTokenId(identifier) + "!!",
           ioe);
+    } finally {
+      if (metrics != null) {
+        metrics.addStoreToken(Time.monotonicNow() - start);
+      }

Review comment:
       A metric will still be recorded when storeToken fails. Is this the same pattern used for other operations?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -825,4 +860,55 @@ protected void syncTokenOwnerStats() {
       addTokenForOwnerStats(id);
     }
   }
+
+  /**
+   * DelegationTokenSecretManagerMetrics tracks token management operations
+   * and publishes them through the metrics interfaces.
+   */
+  @Metrics(about="Delegation token secret manager metrics", context="token")
+  static class DelegationTokenSecretManagerMetrics implements IOStatisticsSource {
+    final static String STORE_TOKEN_STAT = "storeToken";
+    final static String UPDATE_TOKEN_STAT = "updateToken";
+    final static String REMOVE_TOKEN_STAT = "removeToken";
+
+    final MetricsRegistry registry = new MetricsRegistry("DelegationTokenSecretManagerMetrics");

Review comment:
       This registry is unused.

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -555,7 +576,14 @@ public synchronized long renewToken(Token<TokenIdent> token,
       throw new InvalidToken("Renewal request for unknown token "
           + formatTokenId(id));
     }
-    updateToken(id, info);
+    long start = Time.monotonicNow();
+    try {
+      updateToken(id, info);
+    } finally {

Review comment:
       Same issue as above, emitting metrics when there is a failure.




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