[
https://issues.apache.org/jira/browse/MAPREDUCE-7320?focusedWorklogId=557562&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-557562
]
ASF GitHub Bot logged work on MAPREDUCE-7320:
---------------------------------------------
Author: ASF GitHub Bot
Created on: 24/Feb/21 22:33
Start Date: 24/Feb/21 22:33
Worklog Time Spent: 10m
Work Description: jbrennan333 commented on a change in pull request #2722:
URL: https://github.com/apache/hadoop/pull/2722#discussion_r582272551
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
+ * created.
+ * @return the Path of the root directory.
+ */
+ public static Path setupTestRootDir(Class<?> testClass,
+ String defaultTargetRootDir) {
+ // setup the test root directory
+ String targetTestDir =
+ System.getProperty(SYSPROP_TEST_DATA_DIR, defaultTargetRootDir);
+ Path testRootDirPath =
+ new Path(targetTestDir, testClass.getSimpleName());
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
+ testRootDirPath.toString());
Review comment:
I'm a little uneasy about overwriting this property. I think I see why
you are doing it, so getTestDir will do the right thing, but I worry that it
might lead to unexpected behavior because getTestDir is used everywhere. At a
minimum, I think the comment on the function should clearly state that this
changes the property.
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -245,6 +270,18 @@ public static File getTestDir() {
return dir;
}
+ /**
+ * Cleans-up the root directory from the property
+ * {@link #SYSPROP_TEST_DATA_DIR}.
+ *
+ * @return the absolute file of the test root directory.
+ */
+ public static File clearTestRootDir() {
Review comment:
This is only called from setupTestRootDir(), so I don't think it needs
to be a separate public function. I would just do the fullyDelete() call
in-line in setup TestRootDir.
##########
File path:
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
+ private static Path testRootDir;
+
private MiniDFSCluster dfsCluster = null;
- private MiniMRCluster mrCluster = null;
+ private MiniMRClientCluster mrCluster = null;
+
+ protected static void setupClassBase(Class<?> testClass) throws Exception {
+ // setup the test root directory
+ testRootDir = GenericTestUtils.setupTestRootDir(testClass,
+ TEST_ROOT_DEFAULT_PATH);
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
Review comment:
I don't think we should set the property here if we are doing it in
setupTestRootDir().
##########
File path:
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
+ private static Path testRootDir;
+
private MiniDFSCluster dfsCluster = null;
- private MiniMRCluster mrCluster = null;
+ private MiniMRClientCluster mrCluster = null;
+
+ protected static void setupClassBase(Class<?> testClass) throws Exception {
+ // setup the test root directory
+ testRootDir = GenericTestUtils.setupTestRootDir(testClass,
+ TEST_ROOT_DEFAULT_PATH);
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
Review comment:
One question is whether we should get the original value and then reset
it in an afterClass method? Is every unit test run in a separate jvm instance?
Wouldn't want a following test to get the wrong setting.
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
Review comment:
This description is a little unclear. Maybe default relative path to
use if SYSPROP_TEST_DATA_DIR is not set?
##########
File path:
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
Review comment:
This seems redundant, since GenericTestUtils.setupTestRootDir() does the
same thing. Can't we just use GenericTestUtils.DEFAULT_TEST_DATA_DIR? Note
that GenericTestUtils.DEFAULT_TEST_DATA_DIR has the advantage of being correct
in both linux and windows, where the hard-coded strings will only work in linux.
I'm actually not sure you need to pass a default to
GenericTestUtils.setupTestRootDir.
##########
File path:
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/security/ssl/TestEncryptedShuffle.java
##########
@@ -31,37 +30,34 @@
import org.apache.hadoop.mapreduce.MRConfig;
import org.apache.hadoop.security.ssl.KeyStoreTestUtil;
+import org.apache.hadoop.test.GenericTestUtils;
import org.apache.hadoop.util.StringUtils;
import org.apache.hadoop.yarn.conf.YarnConfiguration;
import org.junit.After;
-import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.Assert;
-import java.io.BufferedReader;
import java.io.File;
-import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.Writer;
-import java.net.URL;
public class TestEncryptedShuffle {
- private static final String BASEDIR =
- System.getProperty("test.build.dir", "target/test-dir") + "/" +
- TestEncryptedShuffle.class.getSimpleName();
-
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
Review comment:
Similar to above - just use DEFAULT_TEST_DATA_DIR instead of this. Or
remove it if we decide we don't need the default for setupTestRootDir().
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
Review comment:
I'm actually not sure we need to pass a default here. Everywhere we use
it, it is just using DEFAULT_TEST_DATA_DIR.
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
+ * created.
+ * @return the Path of the root directory.
+ */
+ public static Path setupTestRootDir(Class<?> testClass,
+ String defaultTargetRootDir) {
+ // setup the test root directory
+ String targetTestDir =
+ System.getProperty(SYSPROP_TEST_DATA_DIR, defaultTargetRootDir);
Review comment:
Maybe just use DEFAULT_TEST_DATA_DIR for the default value. Might also
want to add the check for prop.isEmpty() like in getTestDir().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 557562)
Time Spent: 1h 10m (was: 1h)
> ClusterMapReduceTestCase does not clean directories
> ---------------------------------------------------
>
> Key: MAPREDUCE-7320
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-7320
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Reporter: Ahmed Hussein
> Assignee: Ahmed Hussein
> Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> Running Junits that extend {{ClusterMapReduceTestCase}} generate lots of
> directories and folders without cleaning them up.
> For example:
> {code:bash}
> men test -Dtest=TestMRJobClient{code}
> generates the following directories:
> {code:bash}
> - target
> -+ ConfigurableMiniMRCluster_315090884
> -+ ConfigurableMiniMRCluster_1335188990
> -+ ConfigurableMiniMRCluster_1973037511
> -+ test-dir
> -+ dfs
> -+ hadopp-XYZ-01
> -+ hadopp-XYZ-02
> -+ hadopp-XYZ-03
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: mapreduce-issues-h...@hadoop.apache.org
