Activity

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.

Mingliang Liu
added a comment - 14/Apr/16 17:57 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.

Andras Bokor
added a comment - 25/Apr/16 13:04 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?

Andras Bokor
added a comment - 25/Apr/16 14:31 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.

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)?

Andras Bokor
added a comment - 25/Apr/16 17:04 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)?

Thanks Andras Bokor 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.

John Zhuge
added a comment - 25/Apr/16 17:05 Thanks Andras Bokor 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!

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.

John Zhuge
added a comment - 06/May/16 15:33 +1 LGTM. Thanks Andras Bokor 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.

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?

Andras Bokor
added a comment - 09/May/16 14:39 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?