hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [hadoop] steveloughran commented on a change in pull request #2354: HADOOP-17281 Implement FileSystem.listStatusIterator() in S3AFileSystem
Date Tue, 06 Oct 2020 12:59:56 GMT

steveloughran commented on a change in pull request #2354:
URL: https://github.com/apache/hadoop/pull/2354#discussion_r500254718



##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractTestUtils.java
##########
@@ -1520,7 +1521,8 @@ public static TreeScanResults treeWalk(FileSystem fs, Path path)
       while (true) {
         list.add(iterator.next());
       }
-    } catch (NoSuchElementException expected) {
+    } catch (NoSuchElementException | IllegalStateException expected) {

Review comment:
       as discussed: better to fix DirListingIterator to match RemoteIterator API; we can
consider it's failure a bug

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetFileStatusTest.java
##########
@@ -359,16 +357,39 @@ public void testListStatusFile() throws Throwable {
   public void testListStatusIteratorFile() throws Throwable {
     describe("test the listStatusIterator(path) on a file");
     Path f = touchf("listStItrFile");
+
     List<FileStatus> statusList = (List<FileStatus>) iteratorToList(
             getFileSystem().listStatusIterator(f));
-    assertEquals("size of file list returned", 1, statusList.size());
-    assertIsNamedFile(f, statusList.get(0));
+    validateListingForFile(f, statusList, false);
+
     List<FileStatus> statusList2 =
             (List<FileStatus>) iteratorToListThroughNextCallsAlone(
                     getFileSystem().listStatusIterator(f));
-    assertEquals("size of file list returned through next() calls",
-            1, statusList2.size());
-    assertIsNamedFile(f, statusList2.get(0));
+    validateListingForFile(f, statusList2, true);
+  }
+
+  /**
+   * Validate listing result for an input path which is file.
+   * @param f file.
+   * @param statusList list status of a file.
+   * @param nextCallAlone whether the listing generated just using
+   *                      next() calls.
+   */
+  private void validateListingForFile(Path f,
+                                      List<FileStatus> statusList,
+                                      boolean nextCallAlone) {
+    String msg = String.format("size of file list returned using %s should " +
+            "be 1", nextCallAlone ?
+            "next() calls alone" : "hasNext() and next() calls");
+    Assertions.assertThat(statusList)
+            .describedAs(msg)
+            .hasSize(1);
+    Assertions.assertThat(statusList.get(0).getPath().toString())
+            .describedAs("path returned should match with the input path")

Review comment:
       any reason not to leave both as Path and let Path.equals() to do the work? It compares
URIs




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message