hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2962) ZKRMStateStore: Limit the number of znodes under a znode
Date Wed, 01 Mar 2017 23:42:46 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message