[ 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