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-10962. Do not extend from CapacitySchedulerTestBase when not needed. #3459
Conversation
💔 -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 @tomicooler for the PR. I had some minor comments.
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueHelpers.java
Outdated
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueHelpers.java
Outdated
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueHelpers.java
Outdated
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueHelpers.java
Outdated
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerTestBase.java
Show resolved
Hide resolved
26fa2b1
to
81ab52f
Compare
💔 -1 overall
This message was automatically generated. |
81ab52f
to
c2c60c6
Compare
💔 -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.
@tomicooler thanks for the update. Can you please fix the checkstyle issues? Otherwise LGTM, the javac warning seems to be unrelated as you haven't touched that file.
4a4709f
to
3f5d8fc
Compare
💔 -1 overall
This message was automatically generated. |
💔 -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.
@tomicooler thanks for the update, the latest patch looks good to me. The checkstyle warning could be fixed if you create a private constructor like in CapacitySchedulerQueueHelpers.
💔 -1 overall
This message was automatically generated. |
@brumi1024 Unfortunately the private constructor won't work, the TestCapacityScheduler extends this class. |
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.
@tomicooler In this case CapacitySchedulerTestBase just became a utility class, so extending it is pointless (since static methods can't be overridden, and the methods from the utility class are accessible anyway). I suggest removing that extension (for the time being if this will be touched in YARN-10963).
5d061ee
to
cce8248
Compare
I made a utility class called CapacitySchedulerTestUtilities and removed the extension from the TestCapacityScheduler. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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 @tomicooler for working on this. The latest patch LGTM.
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.
Overall looks good to me, please rebase it so it doesn't depend on 10960, which have been just merged.
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerTestUtilities.java
Outdated
Show resolved
Hide resolved
4e31096
to
4507d4d
Compare
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.
@tomicooler thank you, LGTM+1, let's wait for the CI to finish, and then it can be merged.
💔 -1 overall
This message was automatically generated. |
…ded. Change-Id: I7ff0b0977585798f9772f167adfe6a24e52d9737
4507d4d
to
f46e730
Compare
🎊 +1 overall
This message was automatically generated. |
Description of PR
Depends on YARN-10960.
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?