YARN-9048. Add znode hierarchy in Federation ZK State Store. #6016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Sep 4, 2023
| import java.util.List; | ||
| import java.util.TimeZone; | ||
| import java.util.Comparator; | ||
| import java.util.*; |
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code! I will fix it.
.../java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help review this PR again? Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Sep 12, 2023
| @@ -234,6 +273,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<>(5); | |||
There was a problem hiding this comment.
All these hardcoded numbers are dangerous.
Can we define this in a better way?
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code! We define the capacity as 5 because we currently support 0-4 Hierarchies in total. I will improve this part of the code.
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Sep 13, 2023
| @@ -123,16 +126,24 @@ | |||
| import static org.apache.hadoop.util.curator.ZKCuratorManager.getNodePath; | |||
|
|
|||
| /** | |||
| * ZooKeeper implementation of {@link FederationStateStore}. | |||
| * ZooKeeper's implementation of {@link FederationStateStore}. | |||
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code! I will fix it.
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Sep 15, 2023
| String hierarchiesPath = getNodePath(appsZNode, ROUTER_APP_ROOT_HIERARCHIES); | ||
| routerAppRootHierarchies = new HashMap<>(); | ||
| routerAppRootHierarchies.put(0, appsZNode); | ||
| for (int splitIndex = 1; splitIndex <= 4; splitIndex++) { |
There was a problem hiding this comment.
We should make the 1 and 4 constants or depend on some meaningful reference.
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code!
While implementing this part of the code, we referred to the RM's implementation in YARN-2962. The purpose of YARN-2962 is to prevent an excessive number of child nodes under the App's ZNode. This JIRA introduces a hierarchical structure with 5 levels (0-4).
The basic rules are as follows:
hierarchical 0: Represents no hierarchical partitioning, following the original structure where all application information is stored under /app.
hierarchical 1: Represents partitioning based on the last 1 character of the application_id.
hierarchical 2: Represents partitioning based on the last 2 characters of the application_id.
hierarchical 3: Represents partitioning based on the last 3 characters of the application_id.
hierarchical 4: Represents partitioning based on the last 4 characters of the application_id.
Let’s take application_1_0001 as an example:
- Hierarchical1
--/app
---/application_1_000
----/1 (store data)
- Hierarchical2
--/app
---/application_1_00
----/01 (store data)
- Hierarchical3
--/app
---/application_1_0
----/001 (store data)
- Hierarchical4
--/app
---/application_1_
----/0001 (store data)
While implementing this functionality, I aimed to maintain consistency with the RM's implementation, so I chose to adopt the five hierarchicals of partitioning: 0 ~ 4.
There was a problem hiding this comment.
Let's make all this code instead of just having magic numbers around.
| */ | ||
| private void checkRemoveParentAppNode(String appIdPath, int splitIndex) | ||
| throws Exception { | ||
| if (splitIndex != 0) { |
There was a problem hiding this comment.
Let's do early exit to avoid huge nesting for 0.
There was a problem hiding this comment.
I will improve this part of the code.
| return; | ||
| } | ||
| // No apps stored under parent path. | ||
| if (children != null && children.isEmpty()) { |
There was a problem hiding this comment.
I will improve this part of the code.
| } 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()) { |
There was a problem hiding this comment.
I don't think you need the check for isDebugEnabled.
There was a problem hiding this comment.
Thanks for your suggestion! I will remove isDebugEnabled.
| @@ -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); | |||
There was a problem hiding this comment.
I will write this configuration into yarn-site.xml for testing.
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Sep 19, 2023
| nodeUpdatePath = alternatePathInfo.path; | ||
| } else { | ||
| // No alternate path exists. Create path as per configured split index. | ||
| if (appIdNodeSplitIndex != 0) { |
There was a problem hiding this comment.
Move the if into:
} else if (appIdNodeSplitIndex != 0) {
There was a problem hiding this comment.
Thanks for your suggestion! I will fix it.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Oct 3, 2023
| nodeUpdatePath = alternatePathInfo.path; | ||
| } else if (appIdNodeSplitIndex != 0) { | ||
| // No alternate path exists. Create path as per configured split index. | ||
| String rootNode = getSplitAppNodeParent(nodeUpdatePath, appIdNodeSplitIndex); |
There was a problem hiding this comment.
checkstyle is complaining about this line
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code! I will improve this part of the code.
| byte[] data = get(appNodePath); | ||
| LOG.debug("Loading application from znode: {}", appNodePath); | ||
| ApplicationHomeSubCluster appHomeSubCluster = null; | ||
| if (data != null) { |
There was a problem hiding this comment.
You could do early return.
if (data == null) {
return appHomeSubCluster;
}
There was a problem hiding this comment.
I will improve this part of the code.
|
|
||
| private List<ApplicationHomeSubCluster> loadRouterApplications() throws Exception { | ||
| List<ApplicationHomeSubCluster> applicationHomeSubClusters = new ArrayList<>(); | ||
| for (int splitIndex = 0; splitIndex <= 4; splitIndex++) { |
There was a problem hiding this comment.
Again can we have the 4 defined somewhere?
There was a problem hiding this comment.
Thanks for your suggestion! I will define a new variable HIERARCHIES LEVEL
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Oct 4, 2023
| private String routerRMDTSecretManagerRoot; | ||
| private String routerRMDTMasterKeysRootPath; | ||
| private String routerRMDelegationTokensRootPath; | ||
| private String routerRMSequenceNumberPath; | ||
| private String routerRMMasterKeyIdPath; | ||
|
|
||
| private int appIdNodeSplitIndex = 0; | ||
| private final int HIERARCHIES_LEVEL = 4; |
There was a problem hiding this comment.
21: Name 'HIERARCHIES_LEVEL' must match pattern '^[a-z][a-zA-Z0-9]*$'. [MemberName]
I think you need to make it static.
| private String routerRMDTSecretManagerRoot; | ||
| private String routerRMDTMasterKeysRootPath; | ||
| private String routerRMDelegationTokensRootPath; | ||
| private String routerRMSequenceNumberPath; | ||
| private String routerRMMasterKeyIdPath; | ||
|
|
||
| private int appIdNodeSplitIndex = 0; | ||
| private final int HIERARCHIES_LEVEL = 4; |
There was a problem hiding this comment.
Add a comment on what the four are. Probably referring to the hierarchy from before but shorter.
APPLICATION\HIERARCHIES\ID\APP_ID
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Oct 9, 2023
|
@goiri Thank you very much for your help in reviewing the code! |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: YARN-9048. Add znode hierarchy in Federation ZK State Store.
This PR is similar to
YARN-2962. ZKRMStateStore: Limit the number of znodes under a znode. We will provide a similar functionality for the Router, storing applications in a hierarchical manner. We will offer five levels: 0, 1, 2, 3, and 4.We will use two test applications,
application_1_0001andapplication_1_0002, as examples to illustrate how each level appears in the ZK hierarchy.0, we will not modify the ZNode path used for storing applications.1, we will extract the last character of the application and store it inZK.2, we will extract the last two characters of the application and store them inZK.3, we will extract the last three characters of the application and store them inZK.4, we will extract the last four characters of the application and store them inZK.How was this patch tested?
Add Junit Test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?