-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
YARN-11416. FS2CS should use CapacitySchedulerConfiguration in FSQueueConverterBuilder #5320
Conversation
…apacitySchedulerConfiguration object Change-Id: Ifdab821bee6f0f6db4f8b17208d01cf3901820b7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch @susheel-gupta! The benefit of using CapacitySchedulerConfiguration instead of a simple Configuration is to be able to use the helper methods present in the configuration class. This for one benefit makes the code a bit cleaner and handles the defaults and on the other the CS code itself uses these method to check if functionalities are enabled or not. Now if something gets deprecated, or a new property gets added that turns on/off others and this convenience getters handle that we'll immediately see if FS2CS handles it or not. The current approach is just checks if the hard-coded properties are set or not. I've mentioned the convenience methods in comments, can you please check all the CSConf calls to see whether there are getter methods to use?
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Change-Id: I2d6567d4d0cdb149dc9a3dd4bdec3c56e293cbb3
💔 -1 overall
This message was automatically generated. |
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/fair/converter/TestFSConfigToCSConfigConverter.java
Outdated
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/fair/converter/TestFSConfigToCSConfigConverter.java
Outdated
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/fair/converter/TestFSConfigToCSConfigConverter.java
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
...apache/hadoop/yarn/server/resourcemanager/scheduler/fair/converter/TestFSQueueConverter.java
Outdated
Show resolved
Hide resolved
I added some comments where I saw further possibilities for using the convenience get methods beside the fixed ones. I also saw that there are some config settings in the FsQueueConverter class where the CapacitySchedulerConfiguration’s setter methods could be used too, so that way we would have only one place for setting these values. For example here we can use the CapacitySchedulerConfiguration’s setMaximumCapacity() method: Line 167 in a38eb1f
What are your thoughts on that @brumi1024, @susheel-gupta? |
I think the setters can be handled in another separate jira. What do you suggest @p-szucs @brumi1024 ? |
I agree with @p-szucs, it strongly connects to this PR, so it might be best to do it in one go. |
…itySchedulerConfiguration Change-Id: I2d2a9a0d7da17d8fe0e69263743c4c9ecf9b18e0
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @susheel-gupta for the update! Posted some comments.
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
Change-Id: I03760bcdd02ffdd106eb1b92714611b146e0ec67
💔 -1 overall
This message was automatically generated. |
Change-Id: Iec19659eed063cea53a45a548786f090efb87aa7
💔 -1 overall
This message was automatically generated. |
Change-Id: I51e191870f435e79a13143f35a9bec73d564d35d
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @susheel-gupta for the update, I had a few comments. After those we can merge this.
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
Change-Id: If5bac319b2b2fc3cb83f438dc0e19ca09dfcc257
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Thanks @susheel-gupta, the latest state LGTM. Can you please fix the checkstyle issue and that line ending? |
Change-Id: I15a7fca6dbaa93968d59940c09850b4d77c855c3
🎊 +1 overall
This message was automatically generated. |
Change-Id: Ib5d8724e96e4c8540409fe9469a1ad6c525dec0e
🎊 +1 overall
This message was automatically generated. |
Thanks @susheel-gupta for the contribution, @p-szucs for the review, merging to trunk. |
…eConverterBuilder (apache#5320) Co-authored-by: Susheel Gupta <38013283+susheelgupta7@users.noreply.github.com>
…CapacitySchedulerConfiguration object
Change-Id: Ifdab821bee6f0f6db4f8b17208d01cf3901820b7
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?