Details
-
Improvement
-
Status: Resolved
-
Minor
-
Resolution: Fixed
-
2.7.0
-
None
-
Reviewed
Description
MiniDFSCluster should implement AutoCloseable in order to support try-with-resources. It will make test code a little cleaner and more reliable.
Since AutoCloseable is only in Java 1.7 or later, this can not be backported to Hadoop version prior to 2.7.
Attachments
Attachments
- HDFS-10287.01.patch
- 7 kB
- Andras Bokor
- HDFS-10287.02.patch
- 7 kB
- Andras Bokor
- HDFS-10287.03.patch
- 6 kB
- Andras Bokor
Issue Links
- is duplicated by
-
HDFS-10661 Make MiniDFSCluster AutoCloseable
- Resolved
- relates to
-
YARN-4959 MiniYARNCluster should implement AutoCloseable
- Resolved
Activity
SUCCESS: Integrated in Hadoop-trunk-Commit #10130 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10130/)
HDFS-10287. MiniDFSCluster should implement AutoCloseable. Contributed (aajisaka: rev fcde6940e0cbdedb1105007e4857137ecdfa1284)
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
- hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestMiniDFSCluster.java
Committed this to trunk, branch-2, and branch-2.8. Thanks boky01 for the contribution.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 22s | 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 | 8m 20s | trunk passed |
+1 | compile | 1m 0s | trunk passed |
+1 | checkstyle | 0m 32s | trunk passed |
+1 | mvnsite | 1m 2s | trunk passed |
+1 | mvneclipse | 0m 14s | trunk passed |
+1 | findbugs | 1m 53s | trunk passed |
+1 | javadoc | 0m 58s | trunk passed |
+1 | mvninstall | 1m 7s | the patch passed |
+1 | compile | 1m 1s | the patch passed |
+1 | javac | 1m 1s | the patch passed |
+1 | checkstyle | 0m 28s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 206 unchanged - 3 fixed = 206 total (was 209) |
+1 | mvnsite | 1m 8s | the patch passed |
+1 | mvneclipse | 0m 12s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 17s | the patch passed |
+1 | javadoc | 1m 1s | the patch passed |
-1 | unit | 62m 46s | hadoop-hdfs in the patch failed. |
+1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
86m 16s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.hdfs.server.namenode.TestEditLog |
hadoop.hdfs.server.balancer.TestBalancer |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12819288/HDFS-10287.03.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 9216b687ed82 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 / 557a245 |
Default Java | 1.8.0_91 |
findbugs | v3.0.0 |
unit | https://builds.apache.org/job/PreCommit-HDFS-Build/16138/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt |
Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/16138/testReport/ |
modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/16138/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks ajisakaa,
HDFS-10375 removed a duplicated test what was modified by my patch.
I am uploading HDFS-10287.03.patch
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 6s | |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12803063/HDFS-10287.02.patch |
JIRA Issue | |
Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/16118/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
ozawa Can you help me on this? I could use this on one of the issues.
Thanks a lot jzhuge. In this case it seems the patch is good to go. Could you please commit it?
Thanks boky01. Leave the unit test alone to minimize noise for this patch. We can always file a separate jira to clean it up. The unit test failures seem unrelated.
+1 LGTM.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 10s | 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 34s | trunk passed |
+1 | compile | 0m 44s | trunk passed with JDK v1.8.0_91 |
+1 | compile | 0m 45s | trunk passed with JDK v1.7.0_95 |
+1 | checkstyle | 0m 31s | trunk passed |
+1 | mvnsite | 0m 57s | trunk passed |
+1 | mvneclipse | 0m 15s | trunk passed |
+1 | findbugs | 2m 4s | trunk passed |
+1 | javadoc | 1m 9s | trunk passed with JDK v1.8.0_91 |
+1 | javadoc | 1m 48s | trunk passed with JDK v1.7.0_95 |
+1 | mvninstall | 0m 48s | the patch passed |
+1 | compile | 0m 50s | the patch passed with JDK v1.8.0_91 |
+1 | javac | 0m 50s | the patch passed |
+1 | compile | 0m 41s | the patch passed with JDK v1.7.0_95 |
+1 | javac | 0m 41s | the patch passed |
+1 | checkstyle | 0m 27s | hadoop-hdfs-project/hadoop-hdfs: patch generated 0 new + 208 unchanged - 3 fixed = 208 total (was 211) |
+1 | mvnsite | 0m 58s | the patch passed |
+1 | mvneclipse | 0m 12s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 2m 18s | the patch passed |
+1 | javadoc | 1m 5s | the patch passed with JDK v1.8.0_91 |
+1 | javadoc | 1m 55s | the patch passed with JDK v1.7.0_95 |
-1 | unit | 61m 6s | hadoop-hdfs in the patch failed with JDK v1.8.0_91. |
-1 | unit | 56m 46s | hadoop-hdfs in the patch failed with JDK v1.7.0_95. |
+1 | asflicense | 0m 22s | Patch does not generate ASF License warnings. |
145m 32s |
Reason | Tests |
---|---|
JDK v1.8.0_91 Failed junit tests | hadoop.hdfs.TestFileCreationDelete |
hadoop.hdfs.TestCrcCorruption | |
JDK v1.7.0_95 Failed junit tests | hadoop.hdfs.TestFileCreationDelete |
This message was automatically generated.
Test failure seems unrelated. Another JIRA was reported regarding this failure: HDFS-10260
Just as double check triggering build again with HDFS-10287.02.patch
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 11s | 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 5s | trunk passed |
+1 | compile | 0m 44s | trunk passed with JDK v1.8.0_91 |
+1 | compile | 0m 41s | trunk passed with JDK v1.7.0_95 |
+1 | checkstyle | 0m 29s | trunk passed |
+1 | mvnsite | 0m 51s | trunk passed |
+1 | mvneclipse | 0m 13s | trunk passed |
+1 | findbugs | 2m 0s | trunk passed |
+1 | javadoc | 1m 12s | trunk passed with JDK v1.8.0_91 |
+1 | javadoc | 1m 54s | trunk passed with JDK v1.7.0_95 |
+1 | mvninstall | 0m 46s | the patch passed |
+1 | compile | 0m 42s | the patch passed with JDK v1.8.0_91 |
+1 | javac | 0m 42s | the patch passed |
+1 | compile | 0m 38s | the patch passed with JDK v1.7.0_95 |
+1 | javac | 0m 38s | the patch passed |
+1 | checkstyle | 0m 27s | hadoop-hdfs-project/hadoop-hdfs: patch generated 0 new + 208 unchanged - 3 fixed = 208 total (was 211) |
+1 | mvnsite | 0m 49s | the patch passed |
+1 | mvneclipse | 0m 11s | the patch passed |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
+1 | findbugs | 2m 10s | the patch passed |
+1 | javadoc | 1m 5s | the patch passed with JDK v1.8.0_91 |
+1 | javadoc | 1m 44s | the patch passed with JDK v1.7.0_95 |
-1 | unit | 56m 37s | hadoop-hdfs in the patch failed with JDK v1.8.0_91. |
-1 | unit | 53m 58s | hadoop-hdfs in the patch failed with JDK v1.7.0_95. |
+1 | asflicense | 0m 23s | Patch does not generate ASF License warnings. |
136m 57s |
Reason | Tests |
---|---|
JDK v1.8.0_91 Failed junit tests | hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl |
JDK v1.7.0_95 Failed junit tests | hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl |
This message was automatically generated.
I see your point about the tests.
These tests were added with HDFS-2209. Before this patch MiniDFSCluster used only test.build.data system property to determine the base directory. Now the base directory can be set through config object. Based on the comments on HDFS-2209 before the patch to create two instance in the same JVM was not possible (that is not 100% clear to me why. For me it seems setting the system property between the two cluster initialization should work.). It seems to me testDualCluster is a proof of concept that the new feature works. But indeed, the first test proves that the MiniDFSCluster uses hdfs.minidfs.basedir property well.
What is your suggestion? Leave as it is or modify/remove?
+1 LGTM. Thanks boky01 for submitting the patch.
Initially I expected MiniDFSCluster to extend AbstractService just like MiniYARNCluster, but now I am ok with your patch. Just keep it simple until a real use case calls for it.
I think unit test testDualClusters is redundant because testClusterWithoutSystemProperties already proves cluster.getDataDirectory() == getProp(HDFS_MINIDFS_BASEDIR) + "/data". This unit test sets HDFS_MINIDFS_BASEDIR to 2 different values and brings up 2 clusters, of course they will have different data directory.
boky01, do you think MiniDFSCluster should extend AbstractService or even CompositeService?
I added AutoCloseable to the class and I also modified the realated test class as an evidence it works.
Thanks boky01 for creating the patch, however, you comment only applies to MiniYARNCluster not MiniDFSCluster. I closed YARN-4959 accordingly. MiniDFSCluster does NOT extend any class, thus still has the issue.
BTW, your patch updates unit tests to use "try-with-resources" feature. You should create a new jira and upload the patch there.
Thank you for looking into the issue!
You are right. This was linked to YARN-4959 as clone so I did not listen to the class name carefully enough. Either my comment or my patch is not applicable here but for YARN-4959.
Do you think the patch is needed (note that it was done for MiniYARNCluster)?
I attached a patch where I was looking for methods in test classes where we use MiniDFSCluster with finally and changed to try-with-resources style. Please check and let me know what do you think.
MiniDFSCluster supports try-with-resoucres right now:
MiniYARNCluster extends CompositeService
CompositeService extends AbstractService
AbstractService implements Service which extends Closeable which extends AutoCloseable.
So it is not needed to add AutoClosable to MiniDFSCluster.
In addition YARN-4959 is duplicates with this. I think it can be closed
What do you guys think about these?
This will be a good improvement and users of MiniDFSCluster will be grateful. If backward compatibility is a concern and close() is idempotent (seems true), implementing Closeable can be an alternative.
Thanks ajisakaa