hchaverri commented on code in PR #5936:
URL: https://github.com/apache/hadoop/pull/5936#discussion_r1290584223
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -766,6 +774,10 @@ private void removeExpiredToken() throws IOException {
logExpireTokens(expiredTokens);
}
+ protected Map<TokenIdent, DelegationTokenInformation> getTokensForCleanup() {
Review Comment:
Makes sense, I've renamed it
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -415,6 +458,8 @@ public int incrementCurrentKeyId() {
// Token operations in SQL database
protected abstract byte[] selectTokenInfo(int sequenceNum, byte[]
tokenIdentifier)
throws SQLException;
+ protected abstract Map<byte[], byte[]> selectTokenInfos(long
maxModifiedTime, int topResults)
Review Comment:
I thought that having the maxModifiedTime in the parameter list would make
it clear enough, but I'll still rename the method to prevent confusion
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/SQLDelegationTokenSecretManager.java:
##########
@@ -153,6 +163,39 @@ public synchronized TokenIdent
cancelToken(Token<TokenIdent> token,
return super.cancelToken(token, canceller);
}
+ /**
+ * Obtain a list of tokens that will be considered for cleanup, based on the
last
+ * time the token was updated in SQL. This list may include tokens that are
not
+ * expired and should not be deleted (e.g. if the token was last renewed
using a
+ * higher renewal interval).
+ * The number of results is limited to reduce performance impact. Some level
of
+ * contention is expected when multiple routers run cleanup simultaneously.
+ * @return Map of tokens that have not been updated in SQL after the token
renewal
+ * period.
+ */
+ @Override
+ protected Map<TokenIdent, DelegationTokenInformation> getTokensForCleanup() {
+ Map<TokenIdent, DelegationTokenInformation> tokens = new HashMap<>();
+ try {
+ // Query SQL for tokens that haven't been updated after
+ // the last token renewal period.
+ long maxModifiedTime = Time.now() - getTokenRenewInterval();
+ Map<byte[], byte[]> tokenInfoBytesList =
selectTokenInfos(maxModifiedTime,
+ this.maxTokenCleanupResults);
+
+ LOG.info("Found {} tokens for cleanup", tokenInfoBytesList.size());
+ for (Map.Entry<byte[], byte[]> tokenInfoBytes :
tokenInfoBytesList.entrySet()) {
+ TokenIdent tokenIdent = createTokenIdent(tokenInfoBytes.getKey());
+ DelegationTokenInformation tokenInfo =
createTokenInfo(tokenInfoBytes.getValue());
+ tokens.put(tokenIdent, tokenInfo);
+ }
+ } catch (IOException | SQLException e) {
+ LOG.error("Failed to get all tokens in SQL secret manager", e);
Review Comment:
Good catch. I'll change this
--
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]
