Repository: hadoop Updated Branches: refs/heads/branch-2 fa59f4e49 -> ed0d426a8
HADOOP-14170. FileSystemContractBaseTest is not cleaning up test directory clearly. Contributed by Mingliang Liu Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/ed0d426a Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/ed0d426a Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/ed0d426a Branch: refs/heads/branch-2 Commit: ed0d426a88b23965e4188188258a909aa866f012 Parents: fa59f4e Author: Mingliang Liu <[email protected]> Authored: Mon Mar 13 15:19:06 2017 -0700 Committer: Mingliang Liu <[email protected]> Committed: Tue Mar 14 15:00:59 2017 -0700 ---------------------------------------------------------------------- .../hadoop/fs/FileSystemContractBaseTest.java | 209 ++++++++++++------- .../fs/s3a/ITestS3AFileSystemContract.java | 39 +--- 2 files changed, 141 insertions(+), 107 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/ed0d426a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java index 757370e5..4159582 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java @@ -24,8 +24,9 @@ import java.util.ArrayList; import junit.framework.TestCase; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.permission.FsPermission; @@ -43,8 +44,8 @@ import org.apache.hadoop.fs.permission.FsPermission; * </p> */ public abstract class FileSystemContractBaseTest extends TestCase { - private static final Log LOG = - LogFactory.getLog(FileSystemContractBaseTest.class); + private static final Logger LOG = + LoggerFactory.getLogger(FileSystemContractBaseTest.class); protected final static String TEST_UMASK = "062"; protected FileSystem fs; @@ -52,9 +53,46 @@ public abstract class FileSystemContractBaseTest extends TestCase { @Override protected void tearDown() throws Exception { - fs.delete(path("/test"), true); + if (fs != null) { + // some cases use this absolute path + if (rootDirTestEnabled()) { + cleanupDir(path("/FileSystemContractBaseTest")); + } + // others use this relative path against test base directory + cleanupDir(getTestBaseDir()); + } + super.tearDown(); } - + + private void cleanupDir(Path p) { + try { + LOG.info("Deleting " + p); + fs.delete(p, true); + } catch (IOException e) { + LOG.error("Error deleting test dir: " + p, e); + } + } + + /** + * Test base directory for resolving relative test paths. + * + * The default value is /user/$USER/FileSystemContractBaseTest. Subclass may + * set specific test base directory. + */ + protected Path getTestBaseDir() { + return new Path(fs.getWorkingDirectory(), "FileSystemContractBaseTest"); + } + + /** + * For absolute path return the fully qualified path while for relative path + * return the fully qualified path against {@link #getTestBaseDir()}. + */ + protected final Path path(String pathString) { + Path p = new Path(pathString).makeQualified(fs.getUri(), getTestBaseDir()); + LOG.info("Resolving {} -> {}", pathString, p); + return p; + } + protected int getBlockSize() { return 1024; } @@ -67,6 +105,17 @@ public abstract class FileSystemContractBaseTest extends TestCase { return true; } + /** + * Override this if the filesystem does not enable testing root directories. + * + * If this returns true, the test will create and delete test directories and + * files under root directory, which may have side effects, e.g. fail tests + * with PermissionDenied exceptions. + */ + protected boolean rootDirTestEnabled() { + return true; + } + public void testFsStatus() throws Exception { FsStatus fsStatus = fs.getStatus(); assertNotNull(fsStatus); @@ -81,24 +130,24 @@ public abstract class FileSystemContractBaseTest extends TestCase { Path workDir = path(getDefaultWorkingDirectory()); assertEquals(workDir, fs.getWorkingDirectory()); - fs.setWorkingDirectory(path(".")); + fs.setWorkingDirectory(fs.makeQualified(new Path("."))); assertEquals(workDir, fs.getWorkingDirectory()); - fs.setWorkingDirectory(path("..")); + fs.setWorkingDirectory(fs.makeQualified(new Path(".."))); assertEquals(workDir.getParent(), fs.getWorkingDirectory()); - Path relativeDir = path("hadoop"); + Path relativeDir = fs.makeQualified(new Path("testWorkingDirectory")); fs.setWorkingDirectory(relativeDir); assertEquals(relativeDir, fs.getWorkingDirectory()); - Path absoluteDir = path("/test/hadoop"); + Path absoluteDir = path("/FileSystemContractBaseTest/testWorkingDirectory"); fs.setWorkingDirectory(absoluteDir); assertEquals(absoluteDir, fs.getWorkingDirectory()); } public void testMkdirs() throws Exception { - Path testDir = path("/test/hadoop"); + Path testDir = path("testMkdirs"); assertFalse(fs.exists(testDir)); assertFalse(fs.isFile(testDir)); @@ -123,14 +172,15 @@ public abstract class FileSystemContractBaseTest extends TestCase { } public void testMkdirsFailsForSubdirectoryOfExistingFile() throws Exception { - Path testDir = path("/test/hadoop"); + Path testDir = path("testMkdirsFailsForSubdirectoryOfExistingFile"); assertFalse(fs.exists(testDir)); assertTrue(fs.mkdirs(testDir)); assertTrue(fs.exists(testDir)); - createFile(path("/test/hadoop/file")); + createFile(path("testMkdirsFailsForSubdirectoryOfExistingFile/file")); - Path testSubDir = path("/test/hadoop/file/subdir"); + Path testSubDir = + path("testMkdirsFailsForSubdirectoryOfExistingFile/file/subdir"); try { fs.mkdirs(testSubDir); fail("Should throw IOException."); @@ -139,7 +189,8 @@ public abstract class FileSystemContractBaseTest extends TestCase { } assertFalse(fs.exists(testSubDir)); - Path testDeepSubDir = path("/test/hadoop/file/deep/sub/dir"); + Path testDeepSubDir = path( + "testMkdirsFailsForSubdirectoryOfExistingFile/file/deep/sub/dir"); try { fs.mkdirs(testDeepSubDir); fail("Should throw IOException."); @@ -159,7 +210,7 @@ public abstract class FileSystemContractBaseTest extends TestCase { String oldUmask = conf.get(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY); try { conf.set(CommonConfigurationKeys.FS_PERMISSIONS_UMASK_KEY, TEST_UMASK); - final Path dir = new Path("/test/newDir"); + final Path dir = new Path("newDir"); assertTrue(fs.mkdirs(dir, new FsPermission((short)0777))); FileStatus status = fs.getFileStatus(dir); assertTrue(status.isDirectory()); @@ -172,7 +223,8 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testGetFileStatusThrowsExceptionForNonExistentFile() throws Exception { try { - fs.getFileStatus(path("/test/hadoop/file")); + fs.getFileStatus( + path("testGetFileStatusThrowsExceptionForNonExistentFile/file")); fail("Should throw FileNotFoundException"); } catch (FileNotFoundException e) { // expected @@ -181,7 +233,8 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testListStatusThrowsExceptionForNonExistentFile() throws Exception { try { - fs.listStatus(path("/test/hadoop/file")); + fs.listStatus( + path("testListStatusThrowsExceptionForNonExistentFile/file")); fail("Should throw FileNotFoundException"); } catch (FileNotFoundException fnfe) { // expected @@ -189,30 +242,32 @@ public abstract class FileSystemContractBaseTest extends TestCase { } public void testListStatus() throws Exception { - Path[] testDirs = { path("/test/hadoop/a"), - path("/test/hadoop/b"), - path("/test/hadoop/c/1"), }; + final Path[] testDirs = { + path("testListStatus/a"), + path("testListStatus/b"), + path("testListStatus/c/1") + }; assertFalse(fs.exists(testDirs[0])); for (Path path : testDirs) { assertTrue(fs.mkdirs(path)); } - FileStatus[] paths = fs.listStatus(path("/test")); + FileStatus[] paths = fs.listStatus(path(".")); assertEquals(1, paths.length); - assertEquals(path("/test/hadoop"), paths[0].getPath()); + assertEquals(path("testListStatus"), paths[0].getPath()); - paths = fs.listStatus(path("/test/hadoop")); + paths = fs.listStatus(path("testListStatus")); assertEquals(3, paths.length); ArrayList<Path> list = new ArrayList<Path>(); for (FileStatus fileState : paths) { list.add(fileState.getPath()); } - assertTrue(list.contains(path("/test/hadoop/a"))); - assertTrue(list.contains(path("/test/hadoop/b"))); - assertTrue(list.contains(path("/test/hadoop/c"))); + assertTrue(list.contains(path("testListStatus/a"))); + assertTrue(list.contains(path("testListStatus/b"))); + assertTrue(list.contains(path("testListStatus/c"))); - paths = fs.listStatus(path("/test/hadoop/a")); + paths = fs.listStatus(path("testListStatus/a")); assertEquals(0, paths.length); } @@ -243,12 +298,12 @@ public abstract class FileSystemContractBaseTest extends TestCase { * @throws IOException on IO failures */ protected void writeReadAndDelete(int len) throws IOException { - Path path = path("/test/hadoop/file"); + Path path = path("writeReadAndDelete/file"); writeAndRead(path, data, len, false, true); } public void testOverwrite() throws IOException { - Path path = path("/test/hadoop/file"); + Path path = path("testOverwrite/file"); fs.mkdirs(path.getParent()); @@ -274,7 +329,7 @@ public abstract class FileSystemContractBaseTest extends TestCase { } public void testWriteInNonExistentDirectory() throws IOException { - Path path = path("/test/hadoop/file"); + Path path = path("testWriteInNonExistentDirectory/file"); assertFalse("Parent exists", fs.exists(path.getParent())); createFile(path); @@ -284,15 +339,15 @@ public abstract class FileSystemContractBaseTest extends TestCase { } public void testDeleteNonExistentFile() throws IOException { - Path path = path("/test/hadoop/file"); + Path path = path("testDeleteNonExistentFile/file"); assertFalse("Path exists: " + path, fs.exists(path)); assertFalse("No deletion", fs.delete(path, true)); } public void testDeleteRecursively() throws IOException { - Path dir = path("/test/hadoop"); - Path file = path("/test/hadoop/file"); - Path subdir = path("/test/hadoop/subdir"); + Path dir = path("testDeleteRecursively"); + Path file = path("testDeleteRecursively/file"); + Path subdir = path("testDeleteRecursively/subdir"); createFile(file); assertTrue("Created subdir", fs.mkdirs(subdir)); @@ -318,7 +373,7 @@ public abstract class FileSystemContractBaseTest extends TestCase { } public void testDeleteEmptyDirectory() throws IOException { - Path dir = path("/test/hadoop"); + Path dir = path("testDeleteEmptyDirectory"); assertTrue(fs.mkdirs(dir)); assertTrue("Dir exists", fs.exists(dir)); assertTrue("Deleted", fs.delete(dir, false)); @@ -328,26 +383,26 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testRenameNonExistentPath() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/path"); - Path dst = path("/test/new/newpath"); + Path src = path("testRenameNonExistentPath/path"); + Path dst = path("testRenameNonExistentPathNew/newpath"); rename(src, dst, false, false, false); } public void testRenameFileMoveToNonExistentDirectory() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/file"); + Path src = path("testRenameFileMoveToNonExistentDirectory/file"); createFile(src); - Path dst = path("/test/new/newfile"); + Path dst = path("testRenameFileMoveToNonExistentDirectoryNew/newfile"); rename(src, dst, false, true, false); } public void testRenameFileMoveToExistingDirectory() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/file"); + Path src = path("testRenameFileMoveToExistingDirectory/file"); createFile(src); - Path dst = path("/test/new/newfile"); + Path dst = path("testRenameFileMoveToExistingDirectoryNew/newfile"); fs.mkdirs(dst.getParent()); rename(src, dst, true, false, true); } @@ -355,9 +410,9 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testRenameFileAsExistingFile() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/file"); + Path src = path("testRenameFileAsExistingFile/file"); createFile(src); - Path dst = path("/test/new/newfile"); + Path dst = path("testRenameFileAsExistingFileNew/newfile"); createFile(dst); rename(src, dst, false, true, true); } @@ -365,84 +420,84 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testRenameFileAsExistingDirectory() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/file"); + Path src = path("testRenameFileAsExistingDirectory/file"); createFile(src); - Path dst = path("/test/new/newdir"); + Path dst = path("testRenameFileAsExistingDirectoryNew/newdir"); fs.mkdirs(dst); rename(src, dst, true, false, true); assertTrue("Destination changed", - fs.exists(path("/test/new/newdir/file"))); + fs.exists(path("testRenameFileAsExistingDirectoryNew/newdir/file"))); } public void testRenameDirectoryMoveToNonExistentDirectory() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/dir"); + Path src = path("testRenameDirectoryMoveToNonExistentDirectory/dir"); fs.mkdirs(src); - Path dst = path("/test/new/newdir"); + Path dst = path("testRenameDirectoryMoveToNonExistentDirectoryNew/newdir"); rename(src, dst, false, true, false); } public void testRenameDirectoryMoveToExistingDirectory() throws Exception { if (!renameSupported()) return; - - Path src = path("/test/hadoop/dir"); + + Path src = path("testRenameDirectoryMoveToExistingDirectory/dir"); fs.mkdirs(src); - createFile(path("/test/hadoop/dir/file1")); - createFile(path("/test/hadoop/dir/subdir/file2")); - - Path dst = path("/test/new/newdir"); + createFile(path(src + "/file1")); + createFile(path(src + "/subdir/file2")); + + Path dst = path("testRenameDirectoryMoveToExistingDirectoryNew/newdir"); fs.mkdirs(dst.getParent()); rename(src, dst, true, false, true); assertFalse("Nested file1 exists", - fs.exists(path("/test/hadoop/dir/file1"))); + fs.exists(path(src + "/file1"))); assertFalse("Nested file2 exists", - fs.exists(path("/test/hadoop/dir/subdir/file2"))); + fs.exists(path(src + "/subdir/file2"))); assertTrue("Renamed nested file1 exists", - fs.exists(path("/test/new/newdir/file1"))); + fs.exists(path(dst + "/file1"))); assertTrue("Renamed nested exists", - fs.exists(path("/test/new/newdir/subdir/file2"))); + fs.exists(path(dst + "/subdir/file2"))); } public void testRenameDirectoryAsExistingFile() throws Exception { if (!renameSupported()) return; - Path src = path("/test/hadoop/dir"); + Path src = path("testRenameDirectoryAsExistingFile/dir"); fs.mkdirs(src); - Path dst = path("/test/new/newfile"); + Path dst = path("testRenameDirectoryAsExistingFileNew/newfile"); createFile(dst); rename(src, dst, false, true, true); } public void testRenameDirectoryAsExistingDirectory() throws Exception { if (!renameSupported()) return; - - Path src = path("/test/hadoop/dir"); + + Path src = path("testRenameDirectoryAsExistingDirectory/dir"); fs.mkdirs(src); - createFile(path("/test/hadoop/dir/file1")); - createFile(path("/test/hadoop/dir/subdir/file2")); + createFile(path(src + "/file1")); + createFile(path(src + "/subdir/file2")); - Path dst = path("/test/new/newdir"); + Path dst = path("testRenameDirectoryAsExistingDirectoryNew/newdir"); fs.mkdirs(dst); rename(src, dst, true, false, true); assertTrue("Destination changed", - fs.exists(path("/test/new/newdir/dir"))); + fs.exists(path(dst + "/dir"))); assertFalse("Nested file1 exists", - fs.exists(path("/test/hadoop/dir/file1"))); + fs.exists(path(src + "/file1"))); assertFalse("Nested file2 exists", - fs.exists(path("/test/hadoop/dir/subdir/file2"))); + fs.exists(path(src + "r/subdir/file2"))); assertTrue("Renamed nested file1 exists", - fs.exists(path("/test/new/newdir/dir/file1"))); + fs.exists(path(dst + "/dir/file1"))); assertTrue("Renamed nested exists", - fs.exists(path("/test/new/newdir/dir/subdir/file2"))); + fs.exists(path(dst + "/dir/subdir/file2"))); } public void testInputStreamClosedTwice() throws IOException { //HADOOP-4760 according to Closeable#close() closing already-closed //streams should have no effect. - Path src = path("/test/hadoop/file"); + Path src = path("testInputStreamClosedTwice/file"); createFile(src); FSDataInputStream in = fs.open(src); in.close(); @@ -452,17 +507,13 @@ public abstract class FileSystemContractBaseTest extends TestCase { public void testOutputStreamClosedTwice() throws IOException { //HADOOP-4760 according to Closeable#close() closing already-closed //streams should have no effect. - Path src = path("/test/hadoop/file"); + Path src = path("testOutputStreamClosedTwice/file"); FSDataOutputStream out = fs.create(src); out.writeChar('H'); //write some data out.close(); out.close(); } - - protected Path path(String pathString) { - return new Path(pathString).makeQualified(fs); - } - + protected void createFile(Path path) throws IOException { FSDataOutputStream out = fs.create(path); out.write(data, 0, data.length); @@ -490,7 +541,7 @@ public abstract class FileSystemContractBaseTest extends TestCase { byte[] filedata1 = dataset(blockSize * 2, 'A', 26); byte[] filedata2 = dataset(blockSize * 2, 'a', 26); - Path path = path("/test/hadoop/file-overwrite"); + Path path = path("testOverWriteAndRead/file-overwrite"); writeAndRead(path, filedata1, blockSize, true, false); writeAndRead(path, filedata2, blockSize, true, false); writeAndRead(path, filedata1, blockSize * 2, true, false); http://git-wip-us.apache.org/repos/asf/hadoop/blob/ed0d426a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemContract.java ---------------------------------------------------------------------- diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemContract.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemContract.java index f39ec81..6fcf4c7 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemContract.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemContract.java @@ -57,30 +57,13 @@ public class ITestS3AFileSystemContract extends FileSystemContractBaseTest { fs = S3ATestUtils.createTestFileSystem(conf); basePath = fs.makeQualified( - S3ATestUtils.createTestPath(new Path("/s3afilesystemcontract"))); + S3ATestUtils.createTestPath(new Path("s3afilesystemcontract"))); super.setUp(); } - /** - * This path explicitly places all absolute paths under the per-test suite - * path directory; this allows the test to run in parallel. - * @param pathString path string as input - * @return a qualified path string. - */ - protected Path path(String pathString) { - if (pathString.startsWith("/")) { - return fs.makeQualified(new Path(basePath, pathString)); - } else { - return super.path(pathString); - } - } - @Override - protected void tearDown() throws Exception { - if (fs != null) { - fs.delete(basePath, true); - } - super.tearDown(); + public Path getTestBaseDir() { + return basePath; } @Override @@ -94,22 +77,22 @@ public class ITestS3AFileSystemContract extends FileSystemContractBaseTest { return; } - Path src = path("/test/hadoop/dir"); + Path src = path("testRenameDirectoryAsExisting/dir"); fs.mkdirs(src); - createFile(path("/test/hadoop/dir/file1")); - createFile(path("/test/hadoop/dir/subdir/file2")); + createFile(path(src + "/file1")); + createFile(path(src + "/subdir/file2")); - Path dst = path("/test/new/newdir"); + Path dst = path("testRenameDirectoryAsExistingNew/newdir"); fs.mkdirs(dst); rename(src, dst, true, false, true); assertFalse("Nested file1 exists", - fs.exists(path("/test/hadoop/dir/file1"))); + fs.exists(path(src + "/file1"))); assertFalse("Nested file2 exists", - fs.exists(path("/test/hadoop/dir/subdir/file2"))); + fs.exists(path(src + "/subdir/file2"))); assertTrue("Renamed nested file1 exists", - fs.exists(path("/test/new/newdir/file1"))); + fs.exists(path(dst + "/file1"))); assertTrue("Renamed nested exists", - fs.exists(path("/test/new/newdir/subdir/file2"))); + fs.exists(path(dst + "/subdir/file2"))); } // @Override --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
