Details
-
Test
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
-
None
-
None
-
Reviewed
Description
In TestYarnClient, tests commonly wrap methods that throw exceptions in a try catch statement similar to the following:
try { client.submitApplication(context); } catch (Exception e) { Assert.fail("Exception is not expected."); }
This hides useful error messages, and surfaces less helpful ones.
Attachments
Attachments
- YARN-5560.v1.patch
- 13 kB
- Sean Po
- YARN-5560.v2.patch
- 13 kB
- Sean Po
- YARN-5560.v3.patch
- 14 kB
- Sean Po
Activity
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 7m 3s | trunk passed |
+1 | compile | 0m 20s | trunk passed |
+1 | checkstyle | 0m 15s | trunk passed |
+1 | mvnsite | 0m 25s | trunk passed |
+1 | mvneclipse | 0m 15s | trunk passed |
+1 | findbugs | 0m 29s | trunk passed |
+1 | javadoc | 0m 16s | trunk passed |
+1 | mvninstall | 0m 20s | the patch passed |
+1 | compile | 0m 19s | the patch passed |
+1 | javac | 0m 19s | the patch passed |
-1 | checkstyle | 0m 12s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client: The patch generated 1 new + 64 unchanged - 1 fixed = 65 total (was 65) |
+1 | mvnsite | 0m 22s | the patch passed |
+1 | mvneclipse | 0m 12s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 0m 32s | the patch passed |
+1 | javadoc | 0m 11s | the patch passed |
+1 | unit | 15m 44s | hadoop-yarn-client in the patch passed. |
+1 | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
28m 3s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12825513/YARN-5560.v1.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux c571edb34abc 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 1360bd2 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/12898/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-client.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12898/testReport/ |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/12898/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 18s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 6m 43s | trunk passed |
+1 | compile | 0m 20s | trunk passed |
+1 | checkstyle | 0m 14s | trunk passed |
+1 | mvnsite | 0m 25s | trunk passed |
+1 | mvneclipse | 0m 15s | trunk passed |
+1 | findbugs | 0m 29s | trunk passed |
+1 | javadoc | 0m 14s | trunk passed |
+1 | mvninstall | 0m 18s | the patch passed |
+1 | compile | 0m 18s | the patch passed |
+1 | javac | 0m 18s | the patch passed |
+1 | checkstyle | 0m 11s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client: The patch generated 0 new + 64 unchanged - 1 fixed = 64 total (was 65) |
+1 | mvnsite | 0m 22s | the patch passed |
+1 | mvneclipse | 0m 13s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 0m 35s | the patch passed |
+1 | javadoc | 0m 13s | the patch passed |
+1 | unit | 16m 1s | hadoop-yarn-client in the patch passed. |
+1 | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
28m 5s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12825519/YARN-5560.v2.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux b675833cf99f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 1360bd2 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12899/testReport/ |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/12899/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
Thanks, Sean! Patch looks good overall with just one minor nit. Since we're cleaning up bad exception catching practices, this one can be cleaned up as well.
private void waitTillAccepted(YarnClient rmClient, ApplicationId appId, boolean unmanagedApplication) throws Exception { try { [...] } catch (Exception ex) { throw new Exception(ex); }
waitTillAccepted already can throw Exception, so catching an Exception to wrap it in an Exception is redundant.
You're welcome jlowe, thanks for reviewing! V3 of the patch addresses your previous comment.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 18s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 8m 11s | trunk passed |
+1 | compile | 0m 25s | trunk passed |
+1 | checkstyle | 0m 17s | trunk passed |
+1 | mvnsite | 0m 27s | trunk passed |
+1 | mvneclipse | 0m 17s | trunk passed |
+1 | findbugs | 0m 35s | trunk passed |
+1 | javadoc | 0m 17s | trunk passed |
+1 | mvninstall | 0m 24s | the patch passed |
+1 | compile | 0m 21s | the patch passed |
+1 | javac | 0m 21s | the patch passed |
+1 | checkstyle | 0m 13s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client: The patch generated 0 new + 63 unchanged - 2 fixed = 63 total (was 65) |
+1 | mvnsite | 0m 26s | the patch passed |
+1 | mvneclipse | 0m 14s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 0m 40s | the patch passed |
+1 | javadoc | 0m 15s | the patch passed |
+1 | unit | 16m 19s | hadoop-yarn-client in the patch passed. |
+1 | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
30m 33s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12825706/YARN-5560.v3.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 56c2e05d943e 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / cde3a00 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12906/testReport/ |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/12906/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
Thanks, seanpo03! I committed this to trunk, branch-2, and branch-2.8.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10365 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10365/)
YARN-5560. Clean up bad exception catching practices in TestYarnClient. (jlowe: rev 4cbe61407dcb71f099bc7ec6ae87560d786ee714)
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestYarnClient.java
First patch removes occurrences of catch blocks that only invoke Assert.fail in TestYarnClient.