YARN-10904. Investigate: Remove unnecessary fields from AbstractCSQueue or group fields by feature if possible#3551
Conversation
0085f9e to
4f5a1a9
Compare
|
💔 -1 overall
This message was automatically generated. |
4f5a1a9 to
64aa65f
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
72c1cab to
44e9911
Compare
|
💔 -1 overall
This message was automatically generated. |
44e9911 to
a04b2c0
Compare
|
💔 -1 overall
This message was automatically generated. |
9uapaw
left a comment
There was a problem hiding this comment.
Thanks @szilard-nemeth for working on this! I have added some feedback on this. Also, I think it would be a good place to introduce the new QueuePath class in AbstractCSQueue, and also add a getter for it in the CSQueue interface.
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAllocationSettings.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/BasicQueueProperties.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/BasicQueueProperties.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/BasicQueueProperties.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
09063a6 to
adf91a6
Compare
|
Thanks @9uapaw for your review comments. |
|
💔 -1 overall
This message was automatically generated. |
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAllocationSettings.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
Somehow the build is showing weird error messages, for example: On line 746, I have this on my branch: I don't get why this is happening, so just created a patch and uploaded to the jira to trigger the build. Let's see how it goes. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Thanks @szilard-nemeth for the changes, everything looks good to me +1 if the test failures are fixed.
20a78c5 to
1372c05
Compare
|
💔 -1 overall
This message was automatically generated. |
094b280 to
ddf8dc4
Compare
|
💔 -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. |
|
The UT failure is a known flaky, see: https://issues.apache.org/jira/browse/YARN-9450 |
...rc/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
77baad2 to
54f9ef0
Compare
|
💔 -1 overall
This message was automatically generated. |
54f9ef0 to
c167499
Compare
|
💔 -1 overall
This message was automatically generated. |
c167499 to
dea4064
Compare
|
💔 -1 overall
This message was automatically generated. |
dea4064 to
faeb8cb
Compare
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Nit: Is it appropriate to use a public visibility here? It seems more like a private static helper.
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAllocationSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAllocationSettings.java
Outdated
Show resolved
Hide resolved
.../hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAppLifetimeAndLimitSettings.java
Outdated
Show resolved
Hide resolved
.../hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAppLifetimeAndLimitSettings.java
Outdated
Show resolved
Hide resolved
.../hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueAppLifetimeAndLimitSettings.java
Outdated
Show resolved
Hide resolved
|
Thanks @shuzirra, fixed your concerns. |
69a404e to
686fd6d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
shuzirra
left a comment
There was a problem hiding this comment.
@szilard-nemeth thank you for working on this, LGTM+1.
…ue (apache#3551) contributed by Szilard Nemeth
…bstractCSQueue (apache#3551) contributed by Szilard Nemeth (cherry picked from commit d598904) Change-Id: Id7bddde43c83abbb39e6b4f582de25f479459f89 Conflicts: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java Change-Id: Idb0f287a23788144831055b813255ad50bb51a35
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?