-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-10963. Split TestCapacityScheduler by test categories #3460
Conversation
8dbb2ec
to
eda47a7
Compare
💔 -1 overall
This message was automatically generated. |
eda47a7
to
b9eed38
Compare
🎊 +1 overall
This message was automatically generated. |
b9eed38
to
a685fbe
Compare
💔 -1 overall
This message was automatically generated. |
a685fbe
to
6b3d73e
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.
Thanks for working on this @tomicooler. Generally the separations look good to me, but I have the following objection:
- I am not a big fan of inheritance, but in this case I think it would be better to define a common CapacitySchedulerTestBase (which is also the general practice in the codebase) instead of relying on static utility functions. The setUp and tearDown is also repeated and this common base would solve that issue as well.
The CapacitySchedulerTestBase was killed in YARN-10692. The setUp and tearDown phases look similar but the TestCapacitySchedulerNodes doesn't rely on the mockContext for example. The only common member variable would be the resourceManager object. The helper functions could be in the base class, but then they would not be usable by other test classes unless they extend the base. I don't think we would benefit from the base class in this case. |
I agree to this argument, lets keep it that way. +1 from me non-binding, good job! |
7cd0b5a
to
7044d91
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. |
Change-Id: Ib79e0210ad8843edd0f4cd152a4e85d5f34d9ee7
…ated file Change-Id: Ibad16168c1adac149cf3f438ddd0ef3f38b09f18
Change-Id: I760380530ef7db9a3b072a4658248b4f1d9a7990
Change-Id: I15161694c27d65390cf6de1d6482c5ef6df9be1a
Change-Id: I6de5db821e89f048cb0cbbabd7fc883f99301cdf
Change-Id: I64f3e6c1b601940e68bb213d20e3cac5e7652ee5
Change-Id: Ib4cada11c03c5c41f288c0eb7b52e78f489c314e
Change-Id: I4afdc69a342debc770c1fc8b43ffb5e3088e74b6
8a69e16
to
56fd79f
Compare
🎊 +1 overall
This message was automatically generated. |
Thanks @tomicooler for working on this. Also thanks @9uapaw for the review. |
Description of PR
Depends on YARN-10960 YARN-10691 YARN-10692
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?