[
https://issues.apache.org/jira/browse/YARN-9048?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17765851#comment-17765851
]
ASF GitHub Bot commented on YARN-9048:
--------------------------------------
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.
> Add znode hierarchy in Federation ZK State Store
> ------------------------------------------------
>
> Key: YARN-9048
> URL: https://issues.apache.org/jira/browse/YARN-9048
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Bibin Chundatt
> Assignee: Shilun Fan
> Priority: Major
> Labels: pull-request-available
>
> Similar to YARN-2962 consider having hierarchy in ZK federation store for
> applications
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
