omalley commented on a change in pull request #4092:
URL: https://github.com/apache/hadoop/pull/4092#discussion_r836798295
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -429,11 +446,18 @@ private synchronized void removeExpiredKeys() {
byte[] password = createPassword(identifier.getBytes(),
currentKey.getKey());
DelegationTokenInformation tokenInfo = new DelegationTokenInformation(now
+ tokenRenewInterval, password, getTrackingIdIfEnabled(identifier));
+ long start = Time.monotonicNow();
+ boolean success = false;
try {
storeToken(identifier, tokenInfo);
+ success = true;
} catch (IOException ioe) {
LOG.error("Could not store token " + formatTokenId(identifier) + "!!",
ioe);
+ } finally {
Review comment:
The problem with try finally, is that if the code throws an exception
after an exception in the body, the original exception is lost, which makes it
very hard to debug.
Please add the positive call into the body and the failure count in the
ioexception handler. It will mean that we don't count the runtime exceptions,
but I think that is ok in this context.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -825,4 +867,78 @@ 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 {
+ private static final Logger LOG =
LoggerFactory.getLogger(DelegationTokenSecretManagerMetrics.class);
+
+ final static String STORE_TOKEN_STAT = "storeToken";
+ final static String UPDATE_TOKEN_STAT = "updateToken";
+ final static String REMOVE_TOKEN_STAT = "removeToken";
+ final static String TOKEN_FAILURE_STAT = "tokenFailure";
+
+ final MetricsRegistry registry;
+ final IOStatisticsStore ioStatistics;
+
+ @Metric("Rate of storage of delegation tokens and latency (milliseconds)")
+ MutableRate storeToken;
+ @Metric("Rate of update of delegation tokens and latency (milliseconds)")
+ MutableRate updateToken;
+ @Metric("Rate of removal of delegation tokens and latency (milliseconds)")
+ MutableRate removeToken;
+ @Metric("Counter of delegation tokens operation failures")
+ MutableCounterLong tokenFailure;
+
+ static DelegationTokenSecretManagerMetrics create() {
+ return DefaultMetricsSystem.instance().register(new
DelegationTokenSecretManagerMetrics());
+ }
+
+ public DelegationTokenSecretManagerMetrics() {
+ ioStatistics = iostatisticsStore()
+ .withDurationTracking(STORE_TOKEN_STAT, UPDATE_TOKEN_STAT,
REMOVE_TOKEN_STAT)
+ .withCounters(TOKEN_FAILURE_STAT)
+ .build();
+ registry = new MetricsRegistry("DelegationTokenSecretManagerMetrics");
+ LOG.debug("Initialized {}", registry);
+ }
+
+ public void addStoreToken(boolean success, long value) {
+ if (success) {
Review comment:
Rather than have three methods add the success with separate code paths
for true/false. It would be nicer to have a single addFailure.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
