YARN-11084. Introduce new config to specify AM default node-label whe… #4060
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
|
More tests will be attached later. |
|
💔 -1 overall
This message was automatically generated. |
|
If u have time, could you help review it? @szilard-nemeth @9uapaw @virajjasani @jojochuang |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
9uapaw
requested changes
Mar 14, 2022
There was a problem hiding this comment.
Thank you for your contribution @zuston. I think this could be a useful addition, however, YARN properties tend to work a bit different than what you propose here. Since the AM default-node-label-expression is more specific, than the general purpose default-node-label-expression, in my opinion when you set am.default-node-label-expression the following should happen:
- overwrite general purpose default-node-label-expression for the AM container
- if am.default-node-label-expression is not set, then fallback to the general purpose default-node-label-expression
In contrast to how it works in the proposal: - set AM default-node-label-expression only when default-node-label-expression is not set
I think it could be misleading and would make it impossible to define different node label expression for AM and other containers.
| @@ -640,6 +643,12 @@ private RMAppImpl createAndPopulateNewRMApp( | |||
| SchedulerUtils.normalizeAndValidateRequest(amReq, maxAllocation, | |||
| queue, isRecovery, rmContext, null, nodeLabelsEnabled); | |||
|
|
|||
| String amNodeLabel = amReq.getNodeLabelExpression(); | |||
| if (org.apache.commons.lang3.StringUtils.isEmpty(amNodeLabel) | |||
There was a problem hiding this comment.
Please use a simpler package identifier ie. Stringutils.isEmpty, or import isEmpty directly.
|
Thanks for your quick review @9uapaw Agree with your comment. So let's do a priority order user-defined AM node label > AM default-node-label-expression > default general node-label Right? |
|
Updated. Please check it @9uapaw Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
Gentle ping @9uapaw @szilard-nemeth . Thanks ~ |
9uapaw
reviewed
Mar 21, 2022
Comment on lines
3445
to
3446
| When AM of app is not specified any node-labels and this configuration will | ||
| attach the default node-label to AM, which the default of config is disabled. |
There was a problem hiding this comment.
I think it is superfluous to specify an empty value as a yarn-default. It is by default null, if you do not set a default value for it.
There was a problem hiding this comment.
Do u mean that it should not be specified in yarn-default.xml?
But if not, where should be attached detailed config description?
There was a problem hiding this comment.
Sorry I know how to do. Just need to remove the value.
There was a problem hiding this comment.
Yes, I did mean that. You can set a detailed description in the markdown files, in this case: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site/src/site/markdown/NodeLabel.md
There was a problem hiding this comment.
Got it. I will set the detailed description in the nodelabel.md
|
Also, could you address the checkstyle issues as well please? |
|
Updated. Could u help review again? @9uapaw |
9uapaw
reviewed
Mar 21, 2022
Comment on lines
177
to
178
| yarn.resourcemanager.node-labels.am.default-node-label-expression | Besides when ApplicationMaster of application is not specified any node-labels, then the configuration will attach the default node-label to AM. The default of this config is disabled. | ||
|
|
There was a problem hiding this comment.
Could you rephrase it to:
"Overwrites default-node-label-expression only for the ApplicationMaster container. It is disabled by default."
|
💔 -1 overall
This message was automatically generated. |
|
All Done @9uapaw |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for the patch @zuston. I have removed the yarn-default change from the commit because it was unnecessary. |
|
Thanks for your patience and review @9uapaw |
4 tasks
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.
…n not specified
Description of PR
What
When submitting application to Yarn and user don't specify any node-label on AM request and ApplicationSubmissionContext, we hope that Yarn could provide the default AM node-label.
Why
Yarn cluster in our internal company exists on-premise NodeManagers and elastic NodeManagers (which is built on K8s). To prevent application instability due to elastic NM decommission, we hope that the AM of job can be allocated to on-premise NMs.
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?