Details
-
Type:
Bug
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 2.8.0
-
Fix Version/s: 2.9.0, 3.0.0-alpha1
-
Component/s: fairscheduler
-
Labels:
-
Target Version/s:
-
Hadoop Flags:Reviewed
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
Tsuyoshi Ozawa The failed tests cannot be reproduced. Could you rerun them please?
Kai Sasaki, sure, I'll check this on this weekend.
Kai Sasaki thank you for your contribution. I looked over your patch. I have some comments, so could you address them?
private void startResourceManagerWithoutThreshold() {
Why not reusing startResourceManager(threshold) with the threshold larger than 1.0f?
+import org.apache.hadoop.yarn.api.records.*;
Please don't use * import.
Tsuyoshi Ozawa Thank you so much for reviewing!
Why not reusing startResourceManager(threshold) with the threshold larger than 1.0f?
The main different between startResourceManager and startResourceManagerWithoutThreshold seems to be Yarn configuration and initialization of FairScheduler. We can reuse startResourceManager but I'm afraid we cannot be confident not to break existing test code. If a code used by TestFairSchedulerPreemption (in the case startResourceManagerWithoutThreshold) is changed, we cannot say the test code after moving checks the same functionality as before moving. Can we change initialization code?
Kai Sasaki Instead of changing the sequence of initialization, how about changing the name of startResourceManagerWithoutThreshold? I think the name of startResourceManagerWithoutThreshold looks confusing since
the behaviour of the method named startResourceManagerWithoutThreshold() looks to be equals to startResourceManager(1.1f). What do you think?
Tsuyoshi Ozawa It's reasonable. I'll update. Thanks!
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 14s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
| +1 | mvninstall | 6m 42s | trunk passed |
| +1 | compile | 0m 26s | trunk passed with JDK v1.8.0_72 |
| +1 | compile | 0m 29s | trunk passed with JDK v1.7.0_95 |
| +1 | checkstyle | 0m 18s | trunk passed |
| +1 | mvnsite | 0m 35s | trunk passed |
| +1 | mvneclipse | 0m 14s | trunk passed |
| +1 | findbugs | 1m 5s | trunk passed |
| +1 | javadoc | 0m 20s | trunk passed with JDK v1.8.0_72 |
| +1 | javadoc | 0m 25s | trunk passed with JDK v1.7.0_95 |
| +1 | mvninstall | 0m 29s | the patch passed |
| +1 | compile | 0m 23s | the patch passed with JDK v1.8.0_72 |
| +1 | javac | 0m 23s | the patch passed |
| +1 | compile | 0m 25s | the patch passed with JDK v1.7.0_95 |
| +1 | javac | 0m 25s | the patch passed |
| +1 | checkstyle | 0m 15s | the patch passed |
| +1 | mvnsite | 0m 31s | the patch passed |
| +1 | mvneclipse | 0m 13s | the patch passed |
| +1 | whitespace | 0m 0s | Patch has no whitespace issues. |
| +1 | findbugs | 1m 13s | the patch passed |
| +1 | javadoc | 0m 19s | the patch passed with JDK v1.8.0_72 |
| +1 | javadoc | 0m 23s | the patch passed with JDK v1.7.0_95 |
| -1 | unit | 70m 19s | hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.8.0_72. |
| -1 | unit | 71m 41s | hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_95. |
| +1 | asflicense | 0m 17s | Patch does not generate ASF License warnings. |
| 158m 17s |
| Reason | Tests |
|---|---|
| JDK v1.8.0_72 Failed junit tests | hadoop.yarn.server.resourcemanager.TestClientRMTokens |
| hadoop.yarn.server.resourcemanager.TestAMAuthorization | |
| JDK v1.7.0_95 Failed junit tests | hadoop.yarn.server.resourcemanager.TestClientRMTokens |
| hadoop.yarn.server.resourcemanager.TestAMAuthorization |
This message was automatically generated.
Kai Sasaki Thank you for updating. Unfortunatelly, startResourceManagerForPreemptionTest is still confusing because the name of the class is TestFairSchedulerPreemption.
My suggestion is to rename startResourceManager to startResourceManagerWithStubbedFairScheduler, and startResourceManagerForPreemptionTest to startResourceManagerWithRealFairScheduler. Do you have any better idea?
Tsuyoshi Ozawa That naming much better, I also think. I'll update. Thanks!
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 10m 42s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
| +1 | mvninstall | 7m 0s | trunk passed |
| +1 | compile | 0m 30s | trunk passed with JDK v1.8.0_72 |
| +1 | compile | 0m 30s | trunk passed with JDK v1.7.0_95 |
| +1 | checkstyle | 0m 18s | trunk passed |
| +1 | mvnsite | 0m 34s | trunk passed |
| +1 | mvneclipse | 0m 15s | trunk passed |
| +1 | findbugs | 1m 5s | trunk passed |
| +1 | javadoc | 0m 23s | trunk passed with JDK v1.8.0_72 |
| +1 | javadoc | 0m 25s | trunk passed with JDK v1.7.0_95 |
| +1 | mvninstall | 0m 29s | the patch passed |
| +1 | compile | 0m 27s | the patch passed with JDK v1.8.0_72 |
| +1 | javac | 0m 27s | the patch passed |
| +1 | compile | 0m 27s | the patch passed with JDK v1.7.0_95 |
| +1 | javac | 0m 27s | the patch passed |
| +1 | checkstyle | 0m 15s | the patch passed |
| +1 | mvnsite | 0m 32s | the patch passed |
| +1 | mvneclipse | 0m 12s | the patch passed |
| +1 | whitespace | 0m 0s | Patch has no whitespace issues. |
| +1 | findbugs | 1m 15s | the patch passed |
| +1 | javadoc | 0m 19s | the patch passed with JDK v1.8.0_72 |
| +1 | javadoc | 0m 25s | the patch passed with JDK v1.7.0_95 |
| -1 | unit | 67m 37s | hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.8.0_72. |
| -1 | unit | 67m 46s | hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_95. |
| +1 | asflicense | 0m 18s | Patch does not generate ASF License warnings. |
| 162m 43s |
| Reason | Tests |
|---|---|
| JDK v1.8.0_72 Failed junit tests | hadoop.yarn.server.resourcemanager.TestClientRMTokens |
| hadoop.yarn.server.resourcemanager.TestAMAuthorization | |
| JDK v1.7.0_95 Failed junit tests | hadoop.yarn.server.resourcemanager.TestClientRMTokens |
| hadoop.yarn.server.resourcemanager.TestAMAuthorization |
This message was automatically generated.
+1, checking this in.
Note: The failures of TestClientRMTokens and TestAMAuthorization are tracked on HADOOP-12687. It's not related to the patch uploaded here.
Committed this to trunk and branch-2. Thanks Kai Sasaki for your contribution!
FAILURE: Integrated in Hadoop-trunk-Commit #9349 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9349/)
YARN-4648. Move preemption related tests from TestFairScheduler to (ozawa: rev 0e12114c9ccb0c6b16c258227a433b075418796e)
- hadoop-yarn-project/CHANGES.txt
- hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFairSchedulerPreemption.java
- hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFairScheduler.java
Tsuyoshi Ozawa Thanks for reviewing!
YARN-4648This message was automatically generated.