HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem #2354

mukund-thakur · 2020-10-01T06:56:27Z

Ran the new test using ap-south-1 bucket.

O/P-
(ContractTestUtils.java:end(1847)) - Duration of listing 1000 files using listFiles() api with batch size of 10 including 10ms of processing time for each file: 12,223,848,028 nS 2020-10-01 12:19:28,811 [JUnit-testMultiPagesListingPerformanceAndCorrectness] INFO contract.ContractTestUtils (ContractTestUtils.java:end(1847)) - Duration of listing 1000 files using listStatus() api with batch size of 10 including 10ms of processing time for each file: 15,988,037,357 nS 2020-10-01 12:19:41,050 [JUnit-testMultiPagesListingPerformanceAndCorrectness] INFO contract.ContractTestUtils (ContractTestUtils.java:end(1847)) - Duration of listing 1000 files using listStatusIterator() api with batch size of 10 including 10ms of processing time for each file: 12,214,813,052 nS

From the logs we can see that time taken using listStatusIterator() and listFiles() matches and is less than listStatus().

steveloughran

Looks good. Annoying about the return types which force you to do that wrapping/casting. Can't you just forcibly cast the return type of the inner iterator? after all, type erasure means all type info will be lost in the actual compiled binary. I'd prefer that as it will give you automatic passthrough of the IOStatistics stuff.

Add text to filesystem.md, something which:

specifies the result is exactly the same a listStatus, provided no other caller updates the directory during the list
declares that it's not atomic and performance implementations will page
and that if a path isn't there, that fact may not surface until next/hasNext...that is, we do lazy eval for all file IO

We need to similar new contract tests in AbstractContractGetFileStatusTest for all to use

that in a dir with files and subdirectories, you get both returned in the listing
that you can iterate through with next() to failure as well as hasNext/next, and get the same results
listStatusIterator(file) returns the file
listStatusIterator("/") gives you a listing of root (put that in AbstractContractRootDirectoryTest)

And two for changes partway through the iteration

change the directory during a list to add/delete files
deletes the actual path.

These tests can't assert on what will happen, and with paged IO aren't likely to pick up on changes...there just to show it can be done and pick up on any major issues with implementations.

mukund-thakur · 2020-10-05T13:40:57Z

Looks good. Annoying about the return types which force you to do that wrapping/casting. Can't you just forcibly cast the return type of the inner iterator? after all, type erasure means all type info will be lost in the actual compiled binary. I'd prefer that as it will give you automatic passthrough of the IOStatistics stuff.

This is not possible sadly.

Add text to filesystem.md, something which:

specifies the result is exactly the same a listStatus, provided no other caller updates the directory during the list

declares that it's not atomic and performance implementations will page

Done

and that if a path isn't there, that fact may not surface until next/hasNext...that is, we do lazy eval for all file IO

Actually this is not correct, we do throw FNFE if a path is not there, remember we have a check if the listing returns zeror results assuming the path as a directory, we fall back to file checks.

We need to similar new contract tests in AbstractContractGetFileStatusTest for all to use

that in a dir with files and subdirectories, you get both returned in the listing

that you can iterate through with next() to failure as well as hasNext/next, and get the same results

listStatusIterator(file) returns the file

listStatusIterator("/") gives you a listing of root (put that in AbstractContractRootDirectoryTest)

Done

And two for changes partway through the iteration

change the directory during a list to add/delete files

deletes the actual path.

These tests can't assert on what will happen, and with paged IO aren't likely to pick up on changes...there just to show it can be done and pick up on any major issues with implementations.

As discussed not adding these tests.

steveloughran · 2020-10-05T16:10:59Z

stevel and that if a path isn't there, that fact may not surface until next/hasNext...that is, we do lazy eval for all file IO

mukund Actually this is not correct, we do throw FNFE if a path is not there, remember we have a check if the listing returns zeror results assuming the path as a directory, we fall back to file checks.

I'd like the document to leave the option open of a fully async initial listing operation. so the people coding against it now are lined up for future filesystems that do that. Same for any other APIs we add, "late reporting of existence of paths/permissions" is something callers MUST expect

steveloughran

I think I would like the spec to say "potentially async", just to keep people happy, but let's not worry about that too much.

made some minor suggestions about the tests, primarily to use Assertions.assertThat throughout, just for the better reporting.

other than that -all good

steveloughran · 2020-10-05T16:11:56Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+Return an iterator enumerating the `FileStatus` entries under
+a path. This is similar to `listStatus(Path)` except the fact that
+rather than returning an entire list, an iterator is returned.
+The result is exactly the same as listStatus, provided no other caller 


use listStatus() with backticks

steveloughran · 2020-10-05T16:21:29Z

...mon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractTestUtils.java

+  @SuppressWarnings("InfiniteLoopStatement")
+  public static List<? extends FileStatus> iteratorToListThroughNextCallsAlone(
+          RemoteIterator<? extends FileStatus> iterator) throws IOException {
+    ArrayList<FileStatus> list = new ArrayList<>();


same: List as variable type

Actually, it may make sense to type this method

RemoteIterator<T extends FileStatus>

and have

List<T>

and

RemoteIterator<T>

where appropriate

steveloughran · 2020-10-05T16:26:52Z

...op-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractRootDirectoryTest.java

@@ -242,6 +243,13 @@ public void testSimpleRootListing() throws IOException {
            + "listStatus = " + listStatusResult
            + "listFiles = " + listFilesResult,
        fileList.size() <= statuses.length);
+    List<FileStatus> statusList = (List<FileStatus>) iteratorToList(
+            fs.listStatusIterator(root));
+    String listStatusItrRes = join(statusList, "\n");


if you can have both sets of listing as collections, use Assertions.assertThat() as you've done earlier

steveloughran · 2020-10-05T16:28:16Z

...op-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetFileStatusTest.java

+    List<FileStatus> statusList2 =
+            (List<FileStatus>) iteratorToListThroughNextCallsAlone(
+                    getFileSystem().listStatusIterator(f));
+    assertEquals("size of file list returned through next() calls",


AssertionsAssertThat to assert that the list size == 1

steveloughran

LGTM; down to final details

...op-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetFileStatusTest.java

...mon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractTestUtils.java

…NoSuchElementException

mukund-thakur · 2020-10-06T13:38:23Z

...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java

@@ -1107,6 +1107,7 @@ public Void next(final FileSystem fs, final Path p)
    }

    HdfsFileStatus[] partialListing = thisListing.getPartialListing();
+


Please remove this before merging. Was just added to trigger hdfs tests by yetus.

steveloughran · 2020-10-07T09:57:48Z

yay! all the HDFS client tests passed first time! happy!

steveloughran · 2020-10-07T09:58:33Z

OK, +1 on this.

…em (#2354) Contains HADOOP-17300: FileSystem.DirListingIterator.next() call should return NoSuchElementException Contributed by Mukund Thakur Change-Id: I4e7e5c6e295525db9e2de6f416f32bbb81e146d3

mukund-thakur · 2020-10-07T13:58:10Z

Thanks @steveloughran

…S3AFileSystem (apache#2354) Contains HADOOP-17300: FileSystem.DirListingIterator.next() call should return NoSuchElementException Contributed by Mukund Thakur Change-Id: I632956e705e9d4dd4bd692827ad63de775075c9e

HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem

a981d9b

steveloughran self-requested a review October 1, 2020 19:59

steveloughran added enhancement fs/s3 changes related to hadoop-aws; submitter must declare test endpoint labels Oct 1, 2020

steveloughran requested changes Oct 1, 2020

View reviewed changes

HADOOP-17281 Review comments

63d539e

mukund-thakur requested a review from steveloughran October 5, 2020 13:41

steveloughran requested changes Oct 5, 2020

View reviewed changes

HADOOP-17281 some more review comments, checkstyle.

1d658f8

steveloughran reviewed Oct 6, 2020

View reviewed changes

...op-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetFileStatusTest.java Show resolved Hide resolved

...mon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractTestUtils.java Outdated Show resolved Hide resolved

HADOOP-17300 FileSystem.DirListingIterator.next() call should return …

90efdfa

…NoSuchElementException

mukund-thakur commented Oct 6, 2020

View reviewed changes

mukund-thakur requested a review from steveloughran October 7, 2020 05:28

checkstyle fix and removing extra line from DistributedFileSystem

1e493c0

apache deleted a comment from hadoop-yetus Oct 7, 2020

steveloughran merged commit 82522d6 into apache:trunk Oct 7, 2020

apache deleted a comment from hadoop-yetus Oct 7, 2020

steveloughran mentioned this pull request May 12, 2021

HADOOP-17615. ADLS Gen1: Update adls SDK to 2.3.9 #2842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem #2354

HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem #2354

mukund-thakur commented Oct 1, 2020

steveloughran left a comment

mukund-thakur commented Oct 5, 2020

steveloughran commented Oct 5, 2020

steveloughran left a comment

steveloughran Oct 5, 2020

steveloughran Oct 5, 2020

steveloughran Oct 5, 2020

steveloughran Oct 5, 2020

steveloughran left a comment

mukund-thakur Oct 6, 2020 •

edited

Loading

steveloughran commented Oct 7, 2020

steveloughran commented Oct 7, 2020

mukund-thakur commented Oct 7, 2020

		@@ -1107,6 +1107,7 @@ public Void next(final FileSystem fs, final Path p)
		}

		HdfsFileStatus[] partialListing = thisListing.getPartialListing();

HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem #2354

HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem #2354

Conversation

mukund-thakur commented Oct 1, 2020

steveloughran left a comment

Choose a reason for hiding this comment

mukund-thakur commented Oct 5, 2020

steveloughran commented Oct 5, 2020

steveloughran left a comment

Choose a reason for hiding this comment

steveloughran Oct 5, 2020

Choose a reason for hiding this comment

steveloughran Oct 5, 2020

Choose a reason for hiding this comment

steveloughran Oct 5, 2020

Choose a reason for hiding this comment

steveloughran Oct 5, 2020

Choose a reason for hiding this comment

steveloughran left a comment

Choose a reason for hiding this comment

mukund-thakur Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

steveloughran commented Oct 7, 2020

steveloughran commented Oct 7, 2020

mukund-thakur commented Oct 7, 2020

mukund-thakur Oct 6, 2020 •

edited

Loading