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

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

steveloughran commented on code in PR #4092:
URL: https://github.com/apache/hadoop/pull/4092#discussion_r851626510


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -430,10 +447,13 @@ protected synchronized byte[] createPassword(TokenIdent identifier) {
     DelegationTokenInformation tokenInfo = new DelegationTokenInformation(now
         + tokenRenewInterval, password, getTrackingIdIfEnabled(identifier));
     try {
+      long start = Time.monotonicNow();

Review Comment:
   `IOStatisticsStore` is a `DurationTrackerFactory`; you can use it in IOStatisticsBinding invocations to have it track min/mean/max duration of ops. distinguishing success and failure times



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -96,6 +108,10 @@ private String formatTokenId(TokenIdent id) {
    * Access to currentKey is protected by this object lock
    */
   private DelegationKey currentKey;
+  /**
+   * Metrics to track token management operations

Review Comment:
   nit: add a trailing .



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -825,4 +859,68 @@ 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 {

Review Comment:
   make this a DurationTrackerFactory and you can supply duration trackers to IOStatisticsBinding; note also there's a `PairedDurationTrackerFactory` which you could wire up the iostats store. maybe this would be the time/place to add a DurationTrackerFactory for updating hadoop metrics 



##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestDelegationToken.java:
##########
@@ -579,4 +616,55 @@ public void testEmptyToken() throws IOException {
     assertEquals(token1, token2);
     assertEquals(token1.encodeToUrlString(), token2.encodeToUrlString());
   }
+
+  @Test
+  public void testDelegationTokenSecretManagerMetrics() throws Exception {
+    TestDelegationTokenSecretManager dtSecretManager =
+        new TestDelegationTokenSecretManager(24*60*60*1000,
+            10*1000, 1*1000, 60*60*1000);
+    try {
+      dtSecretManager.startThreads();
+
+      Assert.assertEquals(0, dtSecretManager.metrics.storeToken.lastStat().numSamples());

Review Comment:
   this is invoked enough it is worth factoring into its own assertion



##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestDelegationToken.java:
##########
@@ -579,4 +616,55 @@ public void testEmptyToken() throws IOException {
     assertEquals(token1, token2);
     assertEquals(token1.encodeToUrlString(), token2.encodeToUrlString());
   }
+
+  @Test
+  public void testDelegationTokenSecretManagerMetrics() throws Exception {
+    TestDelegationTokenSecretManager dtSecretManager =
+        new TestDelegationTokenSecretManager(24*60*60*1000,
+            10*1000, 1*1000, 60*60*1000);
+    try {
+      dtSecretManager.startThreads();
+
+      Assert.assertEquals(0, dtSecretManager.metrics.storeToken.lastStat().numSamples());
+      final Token<TestDelegationTokenIdentifier> token =
+          generateDelegationToken(dtSecretManager, "SomeUser", "JobTracker");
+      Assert.assertEquals(1, dtSecretManager.metrics.storeToken.lastStat().numSamples());
+
+      Assert.assertEquals(0, dtSecretManager.metrics.updateToken.lastStat().numSamples());
+      dtSecretManager.renewToken(token, "JobTracker");
+      Assert.assertEquals(1, dtSecretManager.metrics.updateToken.lastStat().numSamples());
+
+      Assert.assertEquals(0, dtSecretManager.metrics.removeToken.lastStat().numSamples());
+      dtSecretManager.cancelToken(token, "JobTracker");
+      Assert.assertEquals(1, dtSecretManager.metrics.removeToken.lastStat().numSamples());
+    } finally {
+      dtSecretManager.stopThreads();
+    }
+  }
+
+  @Test
+  public void testDelegationTokenSecretManagerMetricsFailures() throws Exception {
+    TestFailureDelegationTokenSecretManager dtSecretManager = new TestFailureDelegationTokenSecretManager();
+
+    try {
+      dtSecretManager.startThreads();
+
+      final Token<TestDelegationTokenIdentifier> token =
+          generateDelegationToken(dtSecretManager, "SomeUser", "JobTracker");
+
+      dtSecretManager.setThrowError(true);
+
+      Assert.assertEquals(0, dtSecretManager.metrics.tokenFailure.value());
+      generateDelegationToken(dtSecretManager, "SomeUser", "JobTracker");
+      Assert.assertEquals(1, dtSecretManager.metrics.tokenFailure.value());
+
+      LambdaTestUtils.intercept(Exception.class, () -> dtSecretManager.renewToken(token, "JobTracker"));

Review Comment:
   also, if on failure that renew call sleeps for a few hundred millis, you could assert that the duration of the failure was measured/counted. IOStatisticAssertions helps there



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