krishan1390 commented on code in PR #5443:
URL: https://github.com/apache/hadoop/pull/5443#discussion_r1125881513
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -102,26 +102,26 @@ private String formatTokenId(TokenIdent id) {
* Protected by this object lock.
*/
protected int delegationTokenSequenceNumber = 0;
-
+
/**
* Access to allKeys is protected by this object lock
*/
- protected final Map<Integer, DelegationKey> allKeys
+ protected final Map<Integer, DelegationKey> allKeys
= new ConcurrentHashMap<>();
-
+
/**
* Access to currentId is protected by this object lock.
*/
protected int currentId = 0;
/**
* Access to currentKey is protected by this object lock
*/
- private DelegationKey currentKey;
-
- private long keyUpdateInterval;
- private long tokenMaxLifetime;
- private long tokenRemoverScanInterval;
- private long tokenRenewInterval;
+ private volatile DelegationKey currentKey;
Review Comment:
I have set it to volatile to make sure currentKey changes are visible across
threads
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/MemoryFederationStateStore.java:
##########
@@ -524,9 +519,9 @@ public RouterRMTokenResponse
updateStoredToken(RouterRMTokenRequest request)
RouterStoreToken storeToken = request.getRouterStoreToken();
RMDelegationTokenIdentifier tokenIdentifier =
(RMDelegationTokenIdentifier) storeToken.getTokenIdentifier();
- Map<RMDelegationTokenIdentifier, RouterStoreToken> rmDTState =
+ Map<Integer, RouterStoreToken> rmDTState =
Review Comment:
I have replaced the key for the map with the id (sequence number) of the
delegation token rather than the token identifier object itself - because token
identifier objects can be generated repeatedly (as part of different requests)
for the same sequence number
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/secure/TestRouterDelegationTokenSecretManager.java:
##########
@@ -1,201 +0,0 @@
-/**
- * Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
have moved this class to router.security package rather than router.secure
package & refactored tests to test the public methods of router secret manager
rather than testing it internally.
these test cases, validate the database storing / retrieval logic internally
and this makes the test cases independent of the database implementations..
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/FederationDelegationTokenStateStore.java:
##########
@@ -114,37 +114,16 @@ RouterRMTokenResponse
getTokenByRouterStoreToken(RouterRMTokenRequest request)
throws YarnException, IOException;
/**
- * The Router Supports incrementDelegationTokenSeqNum.
+ * Return a new unique integer lookup key for a new delegation token
*
* @return DelegationTokenSeqNum.
*/
- int incrementDelegationTokenSeqNum();
+ int getNewDelegationTokenKey();
/**
- * The Router Supports getDelegationTokenSeqNum.
- *
- * @return DelegationTokenSeqNum.
- */
- int getDelegationTokenSeqNum();
-
- /**
- * The Router Supports setDelegationTokenSeqNum.
- *
- * @param seqNum DelegationTokenSeqNum.
- */
- void setDelegationTokenSeqNum(int seqNum);
-
- /**
- * The Router Supports getCurrentKeyId.
- *
- * @return CurrentKeyId.
- */
- int getCurrentKeyId();
Review Comment:
have removed these methods because they aren't required for a stateless
secret manager. they are only required to support recovery in RM / NN like
systems with only 1 node generating tokens .
with stateless secret manager, there is no explicit recovery mechanism
because by design the secret manager stores all data in database and thus these
methods aren't required.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/MemoryFederationStateStore.java:
##########
@@ -464,13 +461,13 @@ public RouterMasterKeyResponse
storeNewMasterKey(RouterMasterKeyRequest request)
RouterMasterKey masterKey = request.getRouterMasterKey();
DelegationKey delegationKey = getDelegationKeyByMasterKey(masterKey);
- Set<DelegationKey> rmDTMasterKeyState =
routerRMSecretManagerState.getMasterKeyState();
- if (rmDTMasterKeyState.contains(delegationKey)) {
+ Map<Integer, DelegationKey> rmDTMasterKeyState =
routerRMSecretManagerState.getMasterKeyState();
Review Comment:
I have replaced set with a map as we need to lookup objects from their
unique key ids, rather than the object itself
--
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]
