[
https://issues.apache.org/jira/browse/YARN-2962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15891302#comment-15891302
]
Daniel Templeton commented on YARN-2962:
----------------------------------------
Sweet! Thanks for the rebase, [~varun_saxena]. It's been a while, so starting
over with a fresh review. :) Lots of minor points, but no major issues.
# In {{ZKRMStateStore.loadRMAppState()}}, I think {{leafNodePath}} should be
{{parentNodePath}} to be clearer: {{String leafNodePath = getNodePath(appRoot,
childNodeName);}}
# In {{ZKRMStateStore.loadRMAppState()}}, the final _if_ in the _for_ shouldn't
be performed if {{splitIndex}} is 0: {code} if (splitIndex !=
appIdNodeSplitIndex && !appNodeFound) {
// If no loaded app exists for a particular split index and the split
// index for which apps are being loaded is not the one configured, then
// we do not need to keep track of this hierarchy for storing/updating/
// removing app/app attempt znodes.
rmAppRootHierarchies.remove(splitIndex);
}{code} It doesn't hurt anything, though. Maybe best to just add a
comment that says it's OK to remove something that doesn't exist?
# In {{ZKRMStateStore.loadApplicationAttemptState()}}, the {{if
(LOG.isDebugEnabled())}} is superfluous. The arg to the log call doesn't cost
anything to create.
# {{ZKRMStateStore.checkRemoveParentAppNode()}} is missing the description for
the {{@throws}} tag. Same in both {{getLeafAppIdNodePath()}} methods.
# In {{ZKRMStateStore.checkRemoveParentAppNode()}} the last log line isn't
wrapped in an _if_ like all the others
# In {{ZKRMStateStore.getLeafAppIdNodePath()}}, I'd prefer if we didn't do
assignments to the parameters
# In {{ZKRMStateStore.getLeafAppIdNodePath()}} the log line isn't wrapped in an
_if_ like all the others
# This is maybe a bit pedantic, but shouldn't the exception in
{{ZKRMStateStore.storeApplicationAttemptStateInternal()}} be a
{{YarnException}} instead of a {{YarnRuntimeException}}? Unchecked exceptions
should be unexpected. Failing to store an app in ZK is an obvious possibility.
# Seems like the new logic in
{{ZKRMStateStore.storeApplicationAttemptStateInternal()}} and
{{ZKRMStateStore.updateApplicationAttemptStateInternal()}} should be refactored
into another method maybe. You could also use it from
{{removeApplicationAttemptInternal()}}, {{removeApplicationStateInternal()}},
and {{removeApplication()}}
# In {{RMZKStateStore.getSplitAppNodeParent()}}, can we add a comment to
explain why we're subtracting - 1 from the length - split index?
# Instead of {{TestZKRMStateStore.createPath()}}, can we use a Guava {{Joiner}}?
# {{appId}} isn't used in {{TestZKRMStateStore.verifyLoadedAttempt()}}
# Super minor, but in {{TestZKRMStateStore.testAppNodeSplit()}}, it would be
nice to visually separate the app2 code from the app1 code: {code} // Store
attempt associated with app1.
Token<AMRMTokenIdentifier> appAttemptToken1 =
generateAMRMToken(attemptId1, appTokenMgr);
SecretKey clientTokenKey1 =
clientToAMTokenMgr.createMasterKey(attemptId1);
ContainerId containerId1 =
ConverterUtils.toContainerId("container_1352994193343_0001_01_000001");
storeAttempt(store, attemptId1, containerId1.toString(), appAttemptToken1,
clientTokenKey1, dispatcher);
String appAttemptIdStr2 = "appattempt_1352994193343_0001_000002";
ApplicationAttemptId attemptId2 =
ConverterUtils.toApplicationAttemptId(appAttemptIdStr2);
// Store attempt associated with app2.
Token<AMRMTokenIdentifier> appAttemptToken2 =
generateAMRMToken(attemptId2, appTokenMgr);
SecretKey clientTokenKey2 =
clientToAMTokenMgr.createMasterKey(attemptId2);
Credentials attemptCred = new Credentials();
attemptCred.addSecretKey(RMStateStore.AM_CLIENT_TOKEN_MASTER_KEY_NAME,
clientTokenKey2.getEncoded());
ContainerId containerId2 =
ConverterUtils.toContainerId("container_1352994193343_0001_02_000001");
storeAttempt(store, attemptId2, containerId2.toString(), appAttemptToken2,
clientTokenKey2, dispatcher);
{code} Note that the last two statements in the app1 section are actually for
app2.
> ZKRMStateStore: Limit the number of znodes under a znode
> --------------------------------------------------------
>
> Key: YARN-2962
> URL: https://issues.apache.org/jira/browse/YARN-2962
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: resourcemanager
> Affects Versions: 2.6.0
> Reporter: Karthik Kambatla
> Assignee: Varun Saxena
> Priority: Critical
> Attachments: YARN-2962.006.patch, YARN-2962.007.patch,
> YARN-2962.01.patch, YARN-2962.04.patch, YARN-2962.05.patch,
> YARN-2962.2.patch, YARN-2962.3.patch
>
>
> We ran into this issue where we were hitting the default ZK server message
> size configs, primarily because the message had too many znodes even though
> they individually they were all small.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
