[
https://issues.apache.org/jira/browse/YARN-353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13750825#comment-13750825
]
Karthik Kambatla commented on YARN-353:
---------------------------------------
Thanks for the detailed review, [~hitesh].
YARN-353.14.patch rebases against trunk and addresses most of the comments, but
depends on HADOOP-9906.
bq. FS State store uses "fs.rm-state-store" where as ZK uses "zk.state-store" -
this is inconsistent.
YARN-1056 fixes the FSRMStateStore prefix. Now, they are consistent.
bq. Also, the variable names seem a bit inconsistent - should they be
RM_ZK_STATE_STORE* as compared to ZK_RM_STATE_STORE* to match the actual
property names? Though the property name itself has RM defined twice instead of
just once.
The variable names mimic the class names of the RMStateStore implementations. I
believe this reads better.
bq. LOG.debug statement not encapsulated within if isDebugEnabled() in
RMStateStore.java.
Fixed.
bq. Could you add comments so that this piece of code is removed when HA
handling work is done.
Fixed and filed YARN-1099 for the same.
bq. Why are all exceptions being caught instead of an explicit set? If there is
a good reason for all exceptions, why not catch Throwable to capture everything?
bq. The reason for why a connection to ZK failed is never logged. The only
message logged is failed to connect on attempt X and that too only in debug
level.
Fixed.
bq. It seems like "throws Exception" is being used in most places as compared
to known types?
Looked through and fixed where possible.
bq. The tests for basic zk connect/CRUD probably belong in a separate file.
Moved to a separate file.
bq. where is this message being produced? is the RM code validating the format
or ZK, and if ZK, should we be testing this in the first place? Assuming the
test is to validate that we are using the configured value properly, how about
a test for a diff perm from the default and checking that the zkNode has that
permission set.
Fixed through HADOOP-9906 and changes in this patch.
> 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: Karthik Kambatla
> Attachments: YARN-353.10.patch, YARN-353.11.patch, YARN-353.12.patch,
> yarn-353-12-wip.patch, YARN-353.13.patch, YARN-353.14.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
