Jason Lowe commented on YARN-2045:

Thanks for the updated patch!  Comments:

* storeVersion(NMDBSchemaVersion) should be package-private rather than public
* checkVersion should be package-private or potentially private (see next 
* Suggestion: Related to the previous comment, the unit test could use a more 
black-box approach by closing and reopening the state store from scratch after 
updating the version to see if it successfully initializes or not based on the 
updated version.  This would more closely match what happens during normal 
startup and would preclude the need to access the checkVersion method, but it's 
just a suggestion and not a must-fix.
* NMDBSchemaVersionPBImpl should have the Private and Evolving annotations as 
* Minor nit: CURRENT_VERSION_INFO, loadVersion, storeVersion(), and 
getCurrentVersion are protected but private seems more appropriate (except 
loadVersion should be package-private) given the current patch, but you can 
leave as protected if you feel that's a better fit.

> Data persisted in NM should be versioned
> ----------------------------------------
>                 Key: YARN-2045
>                 URL: https://issues.apache.org/jira/browse/YARN-2045
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>    Affects Versions: 2.4.1
>            Reporter: Junping Du
>            Assignee: Junping Du
>         Attachments: YARN-2045-v2.patch, YARN-2045-v3.patch, 
> YARN-2045-v4.patch, YARN-2045.patch
> As a split task from YARN-667, we want to add version info to NM related 
> data, include:
> - NodeManager local LevelDB state
> - NodeManager directory structure

This message was sent by Atlassian JIRA

Reply via email to