goiri commented on code in PR #6016:
URL: https://github.com/apache/hadoop/pull/6016#discussion_r1327784134
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/server/federation/store/impl/TestZookeeperFederationStateStore.java:
##########
@@ -96,6 +96,7 @@ public void before() throws IOException, YarnException {
Configuration conf = new YarnConfiguration();
conf.set(CommonConfigurationKeys.ZK_ADDRESS, connectString);
conf.setInt(YarnConfiguration.FEDERATION_STATESTORE_MAX_APPLICATIONS,
10);
+ conf.setInt(YarnConfiguration.ZK_APPID_NODE_SPLIT_INDEX, 1);
Review Comment:
Can we have better coverage?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##########
@@ -1674,4 +1815,194 @@ public int incrementCurrentKeyId() {
}
return keyIdSeqCounter.getCount();
}
+
+ /**
+ * Get parent app node path based on full path and split index supplied.
+ * @param appIdPath App id path for which parent needs to be returned.
+ * @param splitIndex split index.
+ * @return parent app node path.
+ */
+ private String getSplitAppNodeParent(String appIdPath, int splitIndex) {
+ // Calculated as string upto index (appIdPath Length - split index - 1). We
+ // deduct 1 to exclude path separator.
+ return appIdPath.substring(0, appIdPath.length() - splitIndex - 1);
+ }
+
+ /**
+ * Checks if parent app node has no leaf nodes and if it does not have,
+ * removes it. Called while removing application.
+ *
+ * @param appIdPath path of app id to be removed.
+ * @param splitIndex split index.
+ * @throws Exception if any problem occurs while performing ZK operation.
+ */
+ private void checkRemoveParentAppNode(String appIdPath, int splitIndex)
+ throws Exception {
+ if (splitIndex != 0) {
+ String parentAppNode = getSplitAppNodeParent(appIdPath, splitIndex);
+ List<String> children = null;
+ try {
+ children = getChildren(parentAppNode);
+ } catch (KeeperException.NoNodeException ke) {
+ // It should be fine to swallow this exception as the parent app node
we
+ // intend to delete is already deleted.
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to remove app parent node {} as it does not
exist.",
+ parentAppNode);
+ }
+ return;
+ }
+ // No apps stored under parent path.
+ if (children != null && children.isEmpty()) {
+ try {
+ zkManager.delete(parentAppNode);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No leaf app node exists. Removing parent node {}.",
parentAppNode);
+ }
+ } catch (KeeperException.NotEmptyException ke) {
+ // It should be fine to swallow this exception as the parent app node
+ // has to be deleted only if it has no children. And this node has.
+ if (LOG.isDebugEnabled()) {
Review Comment:
I don't think you need the check for isDebugEnabled.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##########
@@ -1674,4 +1815,194 @@ public int incrementCurrentKeyId() {
}
return keyIdSeqCounter.getCount();
}
+
+ /**
+ * Get parent app node path based on full path and split index supplied.
+ * @param appIdPath App id path for which parent needs to be returned.
+ * @param splitIndex split index.
+ * @return parent app node path.
+ */
+ private String getSplitAppNodeParent(String appIdPath, int splitIndex) {
+ // Calculated as string upto index (appIdPath Length - split index - 1). We
+ // deduct 1 to exclude path separator.
+ return appIdPath.substring(0, appIdPath.length() - splitIndex - 1);
+ }
+
+ /**
+ * Checks if parent app node has no leaf nodes and if it does not have,
+ * removes it. Called while removing application.
+ *
+ * @param appIdPath path of app id to be removed.
+ * @param splitIndex split index.
+ * @throws Exception if any problem occurs while performing ZK operation.
+ */
+ private void checkRemoveParentAppNode(String appIdPath, int splitIndex)
+ throws Exception {
+ if (splitIndex != 0) {
Review Comment:
Let's do early exit to avoid huge nesting for 0.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##########
@@ -234,6 +272,23 @@ public void init(Configuration conf) throws YarnException {
reservationsZNode = getNodePath(baseZNode, ROOT_ZNODE_NAME_RESERVATION);
versionNode = getNodePath(baseZNode, ROOT_ZNODE_NAME_VERSION);
+ String hierarchiesPath = getNodePath(appsZNode,
ROUTER_APP_ROOT_HIERARCHIES);
+ routerAppRootHierarchies = new HashMap<>();
+ routerAppRootHierarchies.put(0, appsZNode);
+ for (int splitIndex = 1; splitIndex <= 4; splitIndex++) {
Review Comment:
We should make the 1 and 4 constants or depend on some meaningful reference.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##########
@@ -1674,4 +1815,194 @@ public int incrementCurrentKeyId() {
}
return keyIdSeqCounter.getCount();
}
+
+ /**
+ * Get parent app node path based on full path and split index supplied.
+ * @param appIdPath App id path for which parent needs to be returned.
+ * @param splitIndex split index.
+ * @return parent app node path.
+ */
+ private String getSplitAppNodeParent(String appIdPath, int splitIndex) {
+ // Calculated as string upto index (appIdPath Length - split index - 1). We
+ // deduct 1 to exclude path separator.
+ return appIdPath.substring(0, appIdPath.length() - splitIndex - 1);
+ }
+
+ /**
+ * Checks if parent app node has no leaf nodes and if it does not have,
+ * removes it. Called while removing application.
+ *
+ * @param appIdPath path of app id to be removed.
+ * @param splitIndex split index.
+ * @throws Exception if any problem occurs while performing ZK operation.
+ */
+ private void checkRemoveParentAppNode(String appIdPath, int splitIndex)
+ throws Exception {
+ if (splitIndex != 0) {
+ String parentAppNode = getSplitAppNodeParent(appIdPath, splitIndex);
+ List<String> children = null;
+ try {
+ children = getChildren(parentAppNode);
+ } catch (KeeperException.NoNodeException ke) {
+ // It should be fine to swallow this exception as the parent app node
we
+ // intend to delete is already deleted.
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Unable to remove app parent node {} as it does not
exist.",
+ parentAppNode);
+ }
+ return;
+ }
+ // No apps stored under parent path.
+ if (children != null && children.isEmpty()) {
Review Comment:
Same you can do early exit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
