[ https://issues.apache.org/jira/browse/YARN-6840?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16165563#comment-16165563 ]
Jonathan Hung edited comment on YARN-6840 at 9/14/17 1:06 AM: -------------------------------------------------------------- Thanks [~leftnoteasy] for the review. Attached 004 addressing most of these comments. Also added a couple of sleeps in TestZKConfigurationStore due to a race condition to fix some test failures. bq. Move following code: To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed. Done bq. Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider. Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there. Unless you meant duplicating the reservation system reinitialization logic inside MutableCSConfigurationProvider#mutateConfiguration? I think this makes more sense, but then we have duplicate code between this and AdminService. bq. In MutableCSConfigurationProvider: It's better to remove:... Done bq. In MutableConfScheduler: Similar to above, it's better to remove:... Done bq. refreshConfiguration -> reloadConfigurationFromStore Done bq. createAndStartZKManager can be merged to rm#getZKManager() Renamed to getAndStartZKManager bq. Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly. Done for ZKConfigurationStore, I left the references in ZKRMStateStore since this class has other direct references to rm object. We can handle this in a separate ticket if you'd like. bq. setResourceManager can be removed and you can pass RMContext ref to initialize. Done bq. What happens if Configuration schedConf passed to a already initialized store? For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store. bq. Could you add Javadocs to following methods: Done was (Author: jhung): Thanks [~leftnoteasy] for the review. Attached 004 addressing most of these comments: bq. Move following code: To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed. Done bq. Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider. Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there. Unless you meant duplicating the reservation system reinitialization logic inside MutableCSConfigurationProvider#mutateConfiguration? I think this makes more sense, but then we have duplicate code between this and AdminService. bq. In MutableCSConfigurationProvider: It's better to remove:... Done bq. In MutableConfScheduler: Similar to above, it's better to remove:... Done bq. refreshConfiguration -> reloadConfigurationFromStore Done bq. createAndStartZKManager can be merged to rm#getZKManager() Renamed to getAndStartZKManager bq. Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly. Done for ZKConfigurationStore, I left the references in ZKRMStateStore since this class has other direct references to rm object. We can handle this in a separate ticket if you'd like. bq. setResourceManager can be removed and you can pass RMContext ref to initialize. Done bq. What happens if Configuration schedConf passed to a already initialized store? For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store. bq. Could you add Javadocs to following methods: Done > Implement zookeeper based store for scheduler configuration updates > ------------------------------------------------------------------- > > Key: YARN-6840 > URL: https://issues.apache.org/jira/browse/YARN-6840 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Wangda Tan > Assignee: Jonathan Hung > Attachments: YARN-6840-YARN-5734.001.patch, > YARN-6840-YARN-5734.002.patch, YARN-6840-YARN-5734.003.patch, > YARN-6840-YARN-5734.004.patch > > > Right now there is only in-memory and leveldb based configuration store > supported. Need one which supports RM HA. -- This message was sent by Atlassian JIRA (v6.4.14#64029) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org