YARN-11708: Setting maximum-application-lifetime using AQCv2 template… #7041
+119
−27
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
…s doesn't apply on the first submitted app
K0K0V0K
suggested changes
Sep 13, 2024
| if (queue == null) { | ||
| QueuePath queuePath = new QueuePath(queueName); | ||
|
|
||
| writeLock.lock(); |
There was a problem hiding this comment.
This write lock should be before the get queue call, to properly handle the race condition, right?
There was a problem hiding this comment.
Thanks for reviewing, yes it should be.
| try { | ||
| queue = queueManager.createQueue(queuePath); | ||
| } catch (YarnException | IOException e) { | ||
| LOG.error("Failed to create queue '{}': ", queueName, e); |
There was a problem hiding this comment.
Suggested change
| LOG.error("Failed to create queue '{}': ", queueName, e); | |
| LOG.error("Failed to create queue " + queueName, e); |
Because the current log line wont log the exception, cause will think that is the 2nd param in the message
K0K0V0K
approved these changes
Sep 13, 2024
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks for the update @susheelgupta7 ! May i ask you to fill the PR description and "How was this patch tested?" parts? |
K0K0V0K
reviewed
Sep 18, 2024
| if (queue == null) { | ||
| String message; | ||
| if (isAmbiguous(queueName)) { | ||
| message = "Application " + app.getApplicationId() |
There was a problem hiding this comment.
I think here we have some code duplication. We can add the
"Application " + app.getApplicationId() + " submitted by user " + app.getUser()
part to the line 3385
| @@ -3366,14 +3367,47 @@ public boolean moveReservedContainer(RMContainer toBeMovedContainer, | |||
|
|
|||
| @Override | |||
| public long checkAndGetApplicationLifetime(String queueName, | |||
There was a problem hiding this comment.
I think instead of add this create new queue logic to the method we could make the getOrCreateQueueFromPlacementContext public and call it before we call the checkAndGetApplicationLifetime method. If i see well this checkAndGetApplicationLifetime used just once in the code
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
K0K0V0K
approved these changes
Sep 19, 2024
|
🎊 +1 overall
This message was automatically generated. |
brumi1024
requested changes
Sep 20, 2024
| @@ -1566,6 +1567,14 @@ public long checkAndGetApplicationLifetime(String queueName, long lifetime) { | |||
| return lifetime; | |||
| } | |||
|
|
|||
| @Override | |||
| public CSQueue getOrCreateQueueFromPlacementContext(ApplicationId | |||
There was a problem hiding this comment.
It shouldn't return a CSQueue, as Fair Scheduler or the Fifo Scheduler do not utilize Capacity Scheduler queues.
| import org.apache.hadoop.yarn.server.resourcemanager.rmcontainer.RMContainer; | ||
| import org.apache.hadoop.yarn.exceptions.YarnException; | ||
| import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CSQueue; |
There was a problem hiding this comment.
YarnScheduler should not rely on one of its implementations' utility classes.
There was a problem hiding this comment.
Thanks for the review. Yes FairScheduler has its own FSQueue class, which is specific to its design.
|
🎊 +1 overall
This message was automatically generated. |
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.
…s doesn't apply on the first submitted app
Description of PR
Setting the maximum-application-lifetime property using AQC v2 templates (yarn.scheduler.capacity.root.test.auto-queue-creation-v2.template.maximum-application-lifetime) doesn't apply to the first submitted application (through which the queue is created), only to the subsequent ones. It should apply to the first application as well. So added a interface to check and create auto-queues if it doesn't exist.
How was this patch tested?
Queue present:
root.test
root.default
root.sample.test
root.queuetest
AQCv2 enabled, maxAppLifetime and defaultAppLifetime are set to 8 sec for "root.test".
Scenarios:
root.test
root.test.user
root.test.alice
root.test.alice.bob
root.sample.test
root.default
root.test.alice.bob.notaqc
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?