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

Devaraj K commented on YARN-1342:
---------------------------------

Thanks for updating the patch. Good work [~jlowe]. Here are some comments for 
the patch. 

1. NodeManager.java

* NMContainerTokenSecretManager gets the nmStore through constructor and 
recover() also gets state as argument which is from nmStore. I think we can get 
the state from nmStore inside recover() instead of getting as an argument.

{code:xml}+      
containerTokenSecretManager.recover(nmStore.loadContainerTokenState());{code}
{code:xml}+        new NMContainerTokenSecretManager(conf, nmStore);{code}

This same applies for NMTokenSecretManagerInNM also.

2. NMLeveldbStateStoreService.java

* Here e.getMessage() may not be required to pass as message since we are 
wrapping the same exception. If we have some custom message we could pass it 
otherwise we can simply use like new IOException(e).
{code:xml}throw new IOException(e.getMessage(), e);{code}
* Can we move the CONTAINER_TOKENS_KEY_PREFIX.length() to outside of the while 
loop?
{code:xml}
String key = fullKey.substring(CONTAINER_TOKENS_KEY_PREFIX.length());
{code}
* Can we make the string *container_* as a constant?
{code:xml}
} else if (key.startsWith("container_")) {
{code}

3. What do you think of having the names like RecoveredContainerTokensState, 
loadContainerTokensState for RecoveredContainerTokenState, 
loadContainerTokesState since these handle more than one ContainerToken?

> Recover container tokens upon nodemanager restart
> -------------------------------------------------
>
>                 Key: YARN-1342
>                 URL: https://issues.apache.org/jira/browse/YARN-1342
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>    Affects Versions: 2.3.0
>            Reporter: Jason Lowe
>            Assignee: Jason Lowe
>         Attachments: YARN-1342.patch, YARN-1342v2.patch, 
> YARN-1342v3-and-YARN-1987.patch, YARN-1342v4.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to