[ 
https://issues.apache.org/jira/browse/YARN-638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13654681#comment-13654681
 ] 

Bikas Saha commented on YARN-638:
---------------------------------


Should HDFS code change to use this (and basically do what it was doing 
earlier). Otherwise the code will still be obfuscated.
{code}
+  protected void removeMasterKey(DelegationKey key) {
+    return;
+  }
{code}

Did not quite get what this is doing?
{code}
         it.remove();
+        if(!e.getValue().equals(currentKey))
+          removeMasterKey(e.getValue());
       }
{code}

Recover secret manager before apps? Apps should be able to assume secret 
manager has valid state.
{code}
     rmAppManager.recover(state);
+
+    // recover RMdelegationTokenSecretManager
+    rmDTSecretManager.recover(state);
{code}

Can we put all secret manager state in one rmDTState object (like appState) and 
then have masterKeyState and delegationTokenState inside it?
{code}
+    // DTIdentifier -> renewDate
+    Map<RMDelegationTokenIdentifier, Long> rmDTState =
+        new HashMap<RMDelegationTokenIdentifier, Long>();
+
+    Set<DelegationKey> rmDTMasterKeyState =
+        new HashSet<DelegationKey>(
{code}

Missing javadoc for new APIs in RMStateStore

loadState()/store secret key impl is missing from filesystem store

Why store redundant rmStateStore when its already available from context?
{code}
+  protected final RMContext rmContext;
+  private RMStateStore rmStateStore;
{code}

Will this just end the current thread or actually stop the RM?
{code}
+    } catch (Exception e) {
+      throw new RuntimeException("Error in removing master key", e);
+    }
{code}

Dont think we should be implementing these methods. There should be explicit 
store.
{code}
+  /**
+   *  remove all expired master keys except current key and store the new key
+   */
+  @Override
+  protected void logUpdateMasterKey(DelegationKey newKey) {
+    try {
+      rmStateStore.storeMasterKey(newKey);
+    } catch (Exception e) {
+      throw new RuntimeException("Error in storing master key", e);
+    }
+  }
+
+  @Override
+  protected void logExpireToken(RMDelegationTokenIdentifier ident)
+      throws IOException {
+    try {
+      rmStateStore.removeRMDelegationToken(ident);
+    } catch (Exception e) {
+      throw new IOException("Error in removing RMDelegationToken", e);
+    }
+  }
{code}

Can just pass in a mock context that returns the test state store
{code}
+  // FOR TEST
+  public void setRMStateStore(RMStateStore rmStore) {
+    this.rmStateStore = rmStore;
+  }
{code}

Is the test ensuring that the recovered tokens can be renewed by the client?

We need to carefully check the threads on which store is being called. We 
cannot block the AsyncDispatcher thread.

                
> Restore RMDelegationTokens after RM Restart
> -------------------------------------------
>
>                 Key: YARN-638
>                 URL: https://issues.apache.org/jira/browse/YARN-638
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Jian He
>            Assignee: Jian He
>         Attachments: YARN-638.1.patch, YARN-638.2.patch, YARN-638.3.patch, 
> YARN-638.4.patch, YARN-638.5.patch
>
>
> This is missed in YARN-581. After RM restart, RMDelegationTokens need to be 
> added both in DelegationTokenRenewer (addressed in YARN-581), and 
> delegationTokenSecretManager

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to