ChenSammi commented on code in PR #3711:
URL: https://github.com/apache/ozone/pull/3711#discussion_r964406538
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -966,6 +1002,107 @@ public RemoteIterator<LocatedFileStatus>
listLocatedStatus(Path f)
return super.listLocatedStatus(f);
}
+ @Override
+ public RemoteIterator<FileStatus> listStatusIterator(Path f)
+ throws IOException {
Review Comment:
Please change the continued line indent from 8 to 4.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
+ }
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+ // Iterator should have an item when dir is not empty
+ it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertEquals("Parent path doesn't match",
+ fileStatus.getPath().toUri().getPath(), parent.toString());
+ }
+ // Iterator on a directory should return all subdirs along with
+ // files, even if there exists a file and sub-dir with the same name.
+ it = ofs.listStatusIterator(parent);
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals(
+ "Iterator did not return all the file status",
+ 2, iCount);
+ // Iterator should return file status for only the
+ // immediate children of a directory.
+ Path file3 = new Path(parent, "dir1/key3");
+ Path file4 = new Path(parent, "dir1/key4");
+ ContractTestUtils.touch(fs, file3);
+ ContractTestUtils.touch(fs, file4);
+ it = ofs.listStatusIterator(parent);
+ iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals("Iterator did not return file status " +
+ "of all the children of the directory", 3, iCount);
+
+ // Cleanup
+ fs.delete(parent, true);
+ }
+
+ /**
+ * Test listStatusIterator operation in a bucket.
+ */
+ @Test
+ public void testListStatusIteratorInBucket() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Path dir1 = new Path(root, "dir1");
+ Path dir12 = new Path(dir1, "dir12");
+ Path dir2 = new Path(root, "dir2");
+ try {
+ fs.mkdirs(dir12);
+ fs.mkdirs(dir2);
+
+ // ListStatus on root should return dir1 (even though /dir1 key does not
+ // exist) and dir2 only. dir12 is not an immediate child of root and
+ // hence should not be listed.
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(root);
+ // Verify that dir12 is not included in the result of the listStatus on
+ // root
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertNotEquals(fileStatus, dir12.toString());
+ }
+ Assert.assertEquals(
+ "FileStatus should return only the immediate children",
+ 2, iCount);
+
+ } finally {
+ // cleanup
+ fs.delete(dir1, true);
+ fs.delete(dir2, true);
+ }
+ }
+
+ /**
+ * Tests listStatusIterator operation on root directory.
+ */
+ @Test
+ public void testListStatusIteratorOnLargeDirectory() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Set<String> paths = new TreeSet<>();
+ int numDirs = LISTING_PAGE_SIZE + LISTING_PAGE_SIZE;
+ try {
+ for (int i = 0; i < numDirs; i++) {
+ Path p = new Path(root, String.valueOf(i));
+ fs.mkdirs(p);
+ paths.add(p.getName());
+ }
+
+ RemoteIterator<FileStatus> iterator = ofs.listStatusIterator(root);
+ int iCount = 0;
+ if (iterator != null) {
+ while (iterator.hasNext()) {
+ FileStatus fileStatus = iterator.next();
+ iCount++;
+ LOG.info("path:{} in FileStatus " + fileStatus.getPath());
+ Assert.assertTrue(paths.contains(fileStatus.getPath().getName()));
+ }
+ }
+ Assert.assertEquals(
+ "Total directories listed do not match the existing directories",
+ numDirs, iCount);
+
+ } finally {
+ // Cleanup
+ for (int i = 0; i < numDirs; i++) {
+ Path p = new Path(root, String.valueOf(i));
+ fs.delete(p, true);
+ }
+ }
+ }
+
+ /**
+ * Tests listStatusIterator on a path with subdirs.
+ */
+ @Test
+ public void testListStatusIteratorOnSubDirs() throws Exception {
+ // Create the following key structure
+ // /dir1/dir11/dir111
+ // /dir1/dir12
+ // /dir1/dir12/file121
+ // /dir2
+ // listStatusIterator on /dir1 should return all its immediated subdirs
only
+ // which are /dir1/dir11 and /dir1/dir12. Super child files/dirs
+ // (/dir1/dir12/file121 and /dir1/dir11/dir111) should not be returned by
+ // listStatusIterator.
+ Path dir1 = new Path(bucketPath, "dir1");
+ Path dir11 = new Path(dir1, "dir11");
+ Path dir111 = new Path(dir11, "dir111");
+ Path dir12 = new Path(dir1, "dir12");
+ Path file121 = new Path(dir12, "file121");
+ Path dir2 = new Path(bucketPath, "dir2");
+ fs.mkdirs(dir111);
+ fs.mkdirs(dir12);
+ ContractTestUtils.touch(fs, file121);
+ fs.mkdirs(dir2);
+
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(dir1);
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertNotEquals(fileStatus, dir12.toString());
+ // Verify that the two children of /dir1
+ // returned by listStatusIterator operation
+ // are /dir1/dir11 and /dir1/dir12.
+ Assert.assertTrue(
+ fileStatus.getPath().toUri().getPath().
+ equals(dir11.toString()) ||
+ fileStatus.getPath().toUri().getPath().
+ equals(dir12.toString()));
+ }
+ Assert.assertEquals(
+ "Iterator should return only the immediate children",
+ 2, iCount);
+
+ // Cleanup
+ fs.delete(dir2, true);
Review Comment:
We should put the cleanup in the finally clause.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
+ }
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+ // Iterator should have an item when dir is not empty
+ it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertEquals("Parent path doesn't match",
+ fileStatus.getPath().toUri().getPath(), parent.toString());
+ }
+ // Iterator on a directory should return all subdirs along with
+ // files, even if there exists a file and sub-dir with the same name.
+ it = ofs.listStatusIterator(parent);
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals(
+ "Iterator did not return all the file status",
+ 2, iCount);
+ // Iterator should return file status for only the
+ // immediate children of a directory.
+ Path file3 = new Path(parent, "dir1/key3");
+ Path file4 = new Path(parent, "dir1/key4");
+ ContractTestUtils.touch(fs, file3);
+ ContractTestUtils.touch(fs, file4);
+ it = ofs.listStatusIterator(parent);
+ iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals("Iterator did not return file status " +
+ "of all the children of the directory", 3, iCount);
+
+ // Cleanup
+ fs.delete(parent, true);
+ }
+
+ /**
+ * Test listStatusIterator operation in a bucket.
+ */
+ @Test
+ public void testListStatusIteratorInBucket() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Path dir1 = new Path(root, "dir1");
+ Path dir12 = new Path(dir1, "dir12");
+ Path dir2 = new Path(root, "dir2");
+ try {
+ fs.mkdirs(dir12);
+ fs.mkdirs(dir2);
+
+ // ListStatus on root should return dir1 (even though /dir1 key does not
+ // exist) and dir2 only. dir12 is not an immediate child of root and
+ // hence should not be listed.
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(root);
+ // Verify that dir12 is not included in the result of the listStatus on
+ // root
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertNotEquals(fileStatus, dir12.toString());
+ }
+ Assert.assertEquals(
+ "FileStatus should return only the immediate children",
+ 2, iCount);
+
+ } finally {
+ // cleanup
+ fs.delete(dir1, true);
+ fs.delete(dir2, true);
+ }
+ }
+
+ /**
+ * Tests listStatusIterator operation on root directory.
+ */
+ @Test
+ public void testListStatusIteratorOnLargeDirectory() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Set<String> paths = new TreeSet<>();
+ int numDirs = LISTING_PAGE_SIZE + LISTING_PAGE_SIZE;
+ try {
+ for (int i = 0; i < numDirs; i++) {
+ Path p = new Path(root, String.valueOf(i));
+ fs.mkdirs(p);
+ paths.add(p.getName());
+ }
+
+ RemoteIterator<FileStatus> iterator = ofs.listStatusIterator(root);
+ int iCount = 0;
+ if (iterator != null) {
+ while (iterator.hasNext()) {
+ FileStatus fileStatus = iterator.next();
+ iCount++;
+ LOG.info("path:{} in FileStatus " + fileStatus.getPath());
Review Comment:
Please remove this log.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
+ }
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+ // Iterator should have an item when dir is not empty
+ it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertEquals("Parent path doesn't match",
+ fileStatus.getPath().toUri().getPath(), parent.toString());
+ }
+ // Iterator on a directory should return all subdirs along with
+ // files, even if there exists a file and sub-dir with the same name.
+ it = ofs.listStatusIterator(parent);
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals(
+ "Iterator did not return all the file status",
+ 2, iCount);
+ // Iterator should return file status for only the
+ // immediate children of a directory.
+ Path file3 = new Path(parent, "dir1/key3");
+ Path file4 = new Path(parent, "dir1/key4");
+ ContractTestUtils.touch(fs, file3);
+ ContractTestUtils.touch(fs, file4);
+ it = ofs.listStatusIterator(parent);
+ iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals("Iterator did not return file status " +
+ "of all the children of the directory", 3, iCount);
+
+ // Cleanup
+ fs.delete(parent, true);
+ }
+
+ /**
+ * Test listStatusIterator operation in a bucket.
+ */
+ @Test
+ public void testListStatusIteratorInBucket() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Path dir1 = new Path(root, "dir1");
+ Path dir12 = new Path(dir1, "dir12");
+ Path dir2 = new Path(root, "dir2");
+ try {
+ fs.mkdirs(dir12);
+ fs.mkdirs(dir2);
+
+ // ListStatus on root should return dir1 (even though /dir1 key does not
+ // exist) and dir2 only. dir12 is not an immediate child of root and
+ // hence should not be listed.
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(root);
+ // Verify that dir12 is not included in the result of the listStatus on
+ // root
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertNotEquals(fileStatus, dir12.toString());
+ }
+ Assert.assertEquals(
+ "FileStatus should return only the immediate children",
+ 2, iCount);
+
+ } finally {
+ // cleanup
+ fs.delete(dir1, true);
+ fs.delete(dir2, true);
+ }
+ }
+
+ /**
+ * Tests listStatusIterator operation on root directory.
+ */
+ @Test
+ public void testListStatusIteratorOnLargeDirectory() throws Exception {
+ Path root = new Path("/" + volumeName + "/" + bucketName);
+ Set<String> paths = new TreeSet<>();
+ int numDirs = LISTING_PAGE_SIZE + LISTING_PAGE_SIZE;
Review Comment:
Can you add more different numDirs value for this test, say 1,
LISTING_PAGE_SIZE, LISTING_PAGE_SIZE + 1, LISTING_PAGE_SIZE -1,
LISTING_PAGE_SIZE + LISTING_PAGE_SIZE/2, etc.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -864,6 +900,107 @@ public RemoteIterator<LocatedFileStatus>
listLocatedStatus(Path f)
return super.listLocatedStatus(f);
}
+ @Override
+ public RemoteIterator<FileStatus> listStatusIterator(Path f)
+ throws IOException {
+ return new OzoneFileStatusIterator<>(f);
+ }
+
+ /**
+ * A private class implementation for iterating list of file status.
+ *
+ * @param <T> the type of the file status
+ */
+ private final class OzoneFileStatusIterator<T extends FileStatus>
Review Comment:
extra space between class and OzoneFileStatusIterator.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
+ }
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+ // Iterator should have an item when dir is not empty
+ it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertEquals("Parent path doesn't match",
+ fileStatus.getPath().toUri().getPath(), parent.toString());
+ }
+ // Iterator on a directory should return all subdirs along with
+ // files, even if there exists a file and sub-dir with the same name.
+ it = ofs.listStatusIterator(parent);
+ int iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals(
+ "Iterator did not return all the file status",
+ 2, iCount);
+ // Iterator should return file status for only the
+ // immediate children of a directory.
+ Path file3 = new Path(parent, "dir1/key3");
+ Path file4 = new Path(parent, "dir1/key4");
+ ContractTestUtils.touch(fs, file3);
+ ContractTestUtils.touch(fs, file4);
+ it = ofs.listStatusIterator(parent);
+ iCount = 0;
+ while (it.hasNext()) {
+ iCount++;
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ }
+ Assert.assertEquals("Iterator did not return file status " +
+ "of all the children of the directory", 3, iCount);
+
+ // Cleanup
+ fs.delete(parent, true);
Review Comment:
Should put the cleanup in finally clause.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
Review Comment:
This line is unreachable if it.hasNext is false.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -635,6 +635,42 @@ workingDir, getUsername())
return statuses.toArray(new FileStatus[0]);
}
+ /**
+ * Get all the file status for input path and startPath.
+ *
+ * @param f
+ * @param startPath
+ * @return
+ * @throws IOException
+ */
+ public FileStatusListing listFileStatus(Path f, String startPath)
Review Comment:
Complete the java doc statement and make it a protected or private class
since it's only called internally.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
Review Comment:
Please add one more test for query an path which doesn't exist.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -387,6 +388,184 @@ public void testListStatus() throws Exception {
fs.delete(parent, true);
}
+ @Test
+ public void testListStatusIteratorWithDir() throws Exception {
+ Path parent = new Path(bucketPath, "testListStatus");
+ Path file1 = new Path(parent, "key1");
+ Path file2 = new Path(parent, "key2");
+
+ // Iterator should have no items when dir is empty
+ RemoteIterator<FileStatus> it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ Assert.assertFalse(it.hasNext());
+ }
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+ // Iterator should have an item when dir is not empty
+ it = ofs.listStatusIterator(bucketPath);
+ while (it.hasNext()) {
+ FileStatus fileStatus = it.next();
+ Assert.assertNotNull(fileStatus);
+ Assert.assertEquals("Parent path doesn't match",
+ fileStatus.getPath().toUri().getPath(), parent.toString());
+ }
+ // Iterator on a directory should return all subdirs along with
+ // files, even if there exists a file and sub-dir with the same name.
Review Comment:
Same name should not be allowed. please remove this statement.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
