[ https://issues.apache.org/jira/browse/YARN-353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13734473#comment-13734473 ]
Karthik Kambatla commented on YARN-353: --------------------------------------- bq. Identation still off in yarn-default.xml bq. hadoop-yarn-server-resourcemanager/pom.xml is still not fixed - contains the version info. LOG.debug() should be wrapped within "if LOG.isDebugEnabled() {}" bq. In "abstract class ZKAction<T>", member variables should be final. bq. findbugs issue - zkSessionTimeout does not seem to be an issue but zkClient is something which is read/modified multiple times in various functions. Which functions are the ones that are/cannot be synchronized that access zkClient? bq. "os.close();" - close calls should be in a finally block ( in multiple places ). bq. "LOG.info("Error in storing " + dtSequenceNumberPath);" - change to LOG.info("Error in storing " + dtSequenceNumberPath, e);" bq. "LOG.info("Created new connection for " + this);" --> logging 'this'? is there a toString() function? bq. How is a connection failure to zk handled? i.e. getNewZooKeeper() throws an exception. Does the RM fail/shutdown? Is the connection retried at a later point? bq. what if the first line throws an exception saying node exists but the other nodes are not created? Shouldn't each call be in its own try catch block? Or should the create function be changed to accept a parameter which when set causes the function to ignore node exists errors? bq. For deleteWithRetries, the return code of exists() could be checked if a delete is required or not. Fixed. bq. When creating a Zookeeper object, ZK apis support a base root path and all operations are done relative to the base root path? Any reason why we are not using that approach by initializing zk with zkRootNodePath ? We are calling createWithRetries on multiple roots bq. why the catch and re-throw? ( in multiple places ) The HA code might want to deal with those scenarios differently. Given we are on the verge of implement ZK-based HA, I think it is okay to leave these as they currently are. bq. validation only for non-null and not a valid format? While it is a good idea to check for a valid format, the current behavior is how RM etc. deal with host:port arguments bq. Not sure if a default value of "<!-value>127.0.0.1:2181</value->" should be mentioned in yarn-default.xml. Changed the comment to be host:port. Leaving it there makes it easier for users. > Add Zookeeper-based store implementation for RMStateStore > --------------------------------------------------------- > > Key: YARN-353 > URL: https://issues.apache.org/jira/browse/YARN-353 > Project: Hadoop YARN > Issue Type: Sub-task > Components: resourcemanager > Reporter: Hitesh Shah > Assignee: Bikas Saha > Attachments: YARN-353.10.patch, YARN-353.11.patch, YARN-353.12.patch, > yarn-353-12-wip.patch, YARN-353.1.patch, YARN-353.2.patch, YARN-353.3.patch, > YARN-353.4.patch, YARN-353.5.patch, YARN-353.6.patch, YARN-353.7.patch, > YARN-353.8.patch, YARN-353.9.patch > > > Add store that write RM state data to ZK -- 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