YARN-11584. Safely fail on leaf queue with empty name #6148
Conversation
This file contains hidden or 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
Prevents auto-creation of a leaf queue with an empty string as the shortname in CapacityScheduler, which caused FATAL exception in RM.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures seem intermittent and unrelated to this change. Same tests pass on my local machine. |
K0K0V0K
approved these changes
Oct 11, 2023
brumi1024
reviewed
Oct 18, 2023
There was a problem hiding this comment.
Thanks @bgoerlitz for the patch! I was wondering about a possible simplification, see my comments.
Comment on lines
512
to
517
| if (StringUtils.isEmpty(leafQueueName)) { | ||
| throw new SchedulerDynamicEditException( | ||
| "Trying to create new queue=" + queue.getFullPath() | ||
| + ", which has empty leaf shortname."); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this part is necessary. QueueManager.createQueue(QueuePath) is only called from CapacityScheduler.getOrCreateQueueFromPlacementContext (at least from production code), and it has the following check right before this call:
//we need to make sure there are no empty path parts present
if (queuePath.hasEmptyPart()) {
LOG.error("Application submitted to invalid path due to empty parts: " +
"'{}'", queuePath);
return null;
}
So essentially we should not be able to reach this from production code, only from tests.
There was a problem hiding this comment.
Agreed with this. This path currently does fail due to the fact that QueuePath.hasEmptyParts() does not actually return true for an empty leaf name, but the added unit test for QueueManager.createQueue(QueuePath) is not necessary, so I will remove that in the next commit.
Comment on lines
493
to
497
| if (StringUtils.isEmpty(queueShortName)) { | ||
| throw new SchedulerDynamicEditException( | ||
| "Trying to create new queue=" + childQueuePath | ||
| + ", which has empty leaf shortname."); | ||
| } |
There was a problem hiding this comment.
Not sure about this. Going up the callchain:
AbstractParentQueue.createNewQueueis only called fromParentQueue.addDynamicChildQueue,- which (through another method) is called from
CapacitySchedulerQueueManager.createAutoQueue, - which is called from
CapacitySchedulerQueueManager.createQueue, where my second comment should apply
There is one difference: upon determining whether we're using legacy auto creation or the flexible one we're using different methods, and the flexible auto queue creation (method: CapacitySchedulerQueueManager.createQueue .createAutoQueue(queue)) manipulates the QueuePath using List<String> parentsToCreate = determineMissingParents(queue). If an empty string part can be added to the list, or can slip through I think it's probably there, which I think should be a better place for these checks.
|
Thanks for the comments @brumi1024. In fact it turns out that |
… for empty leaf shortname As QueuePath.hasEmptyPart() is already called in previously patched code paths, the explicit checks for empty leaf shortname are removed. Additionally, the Unit Test for CapacityScheudulerQueueManager.createQueue(QueuePath) is unnecessary, so was removed.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@bgoerlitz thanks for the update, the latest change LGTM. Merging to trunk. @K0K0V0K thanks for the review. |
brumi1024
approved these changes
Oct 31, 2023
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.
Prevents auto-creation of a leaf queue with an empty string as the shortname in CapacityScheduler, which caused FATAL exception in RM.
Description of PR
Auto-creation of a leaf queue does not check that the leaf queue's shortname is non-empty. This PR adds checks in both legacy and AQCv2 paths to catch this scenario and safely throw an exception.
How was this patch tested?
Two new unit tests added.