krishan1390 commented on code in PR #5443:
URL: https://github.com/apache/hadoop/pull/5443#discussion_r1135307848
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/security/RouterDelegationTokenSecretManager.java:
##########
@@ -91,283 +117,173 @@ private boolean shouldIgnoreException(Exception e) {
* @param newKey DelegationKey
*/
@Override
- public void storeNewMasterKey(DelegationKey newKey) {
+ protected void storeNewMasterKey(DelegationKey newKey) throws IOException {
try {
federationFacade.storeNewMasterKey(newKey);
- } catch (Exception e) {
- if (!shouldIgnoreException(e)) {
- LOG.error("Error in storing master key with KeyID: {}.",
newKey.getKeyId());
- ExitUtil.terminate(1, e);
- }
+ } catch (YarnException e) {
+ e.printStackTrace();
+ throw new IOException(e); // Wrap YarnException as an IOException to
adhere to storeNewMasterKey contract
}
}
/**
- * The Router Supports Remove the master key.
- * During this Process, Facade will call the specific StateStore to remove
the MasterKey.
- *
- * @param delegationKey DelegationKey
+ * no-op as expiry of stored keys is upto the state store in a stateless
secret manager
*/
@Override
public void removeStoredMasterKey(DelegationKey delegationKey) {
- try {
- federationFacade.removeStoredMasterKey(delegationKey);
- } catch (Exception e) {
- if (!shouldIgnoreException(e)) {
- LOG.error("Error in removing master key with KeyID: {}.",
delegationKey.getKeyId());
- ExitUtil.terminate(1, e);
- }
- }
+
}
/**
- * The Router Supports Store new Token.
- *
- * @param identifier RMDelegationToken
- * @param renewDate renewDate
- * @throws IOException IO exception occurred.
+ * no-op as we are storing entire token with info in storeToken()
*/
@Override
- public void storeNewToken(RMDelegationTokenIdentifier identifier,
- long renewDate) throws IOException {
- try {
- federationFacade.storeNewToken(identifier, renewDate);
Review Comment:
yea thats not required. was planning to raise a seperate PR for changes in
SQLFederationStateStore - will keep this PR focused on changes required for
stateless delegation token management
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -434,16 +446,17 @@ private void updateCurrentKey() throws IOException {
DelegationKey newKey = new DelegationKey(newCurrentId, System
.currentTimeMillis()
+ keyUpdateInterval + tokenMaxLifetime, generateSecret());
- //Log must be invoked outside the lock on 'this'
- logUpdateMasterKey(newKey);
synchronized (this) {
+ storeDelegationKey(newKey);
Review Comment:
actually even updating currentKey needn't be in synchronized block because
currentKey is volatile so the update will be visible.
I had originally removed synchronized here but added it back to avoid the
change in this PR and to make it seperately.
wanted to keep the PR focused specifically on whats required for reliable
stateless setup - in this particular case, trying to store in DB first before
updating currentKey
--
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]
