adoroszlai commented on code in PR #6416:
URL: https://github.com/apache/ozone/pull/6416#discussion_r1535095279


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -753,6 +754,33 @@ public void testLinkBucketOrphan() throws Exception {
     }
   }
 
+  @Test
+  public void testListBucket() throws Exception {
+    final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
+    OzoneConfiguration clientConf =
+            getClientConfForOFS(hostPrefix, cluster.getConf());
+    int pageSize = 20;
+    clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
+    OzoneFsShell shell = new OzoneFsShell(clientConf);
+
+    String volName = "testlistbucket";
+    int numBuckets = pageSize;
+
+    try {
+      generateBuckets("/" + volName, numBuckets);
+      int res;

Review Comment:
   nit: join with value assignment.



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -753,6 +754,33 @@ public void testLinkBucketOrphan() throws Exception {
     }
   }
 
+  @Test
+  public void testListBucket() throws Exception {
+    final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
+    OzoneConfiguration clientConf =
+            getClientConfForOFS(hostPrefix, cluster.getConf());
+    int pageSize = 20;
+    clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);

Review Comment:
   `FileSystem` may already be cached, so updated config may not be applied.
   
   ```suggestion
       clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
       URI uri = FileSystem.getDefaultUri(clientConf);
       clientConf.setBoolean(String.format("fs.%s.impl.disable.cache", 
uri.getScheme()), true);
   ```
   
   (needs `import java.net.URI`, too)



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -753,6 +754,33 @@ public void testLinkBucketOrphan() throws Exception {
     }
   }
 
+  @Test
+  public void testListBucket() throws Exception {
+    final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
+    OzoneConfiguration clientConf =
+            getClientConfForOFS(hostPrefix, cluster.getConf());
+    int pageSize = 20;
+    clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
+    OzoneFsShell shell = new OzoneFsShell(clientConf);
+
+    String volName = "testlistbucket";
+    int numBuckets = pageSize;
+
+    try {
+      generateBuckets("/" + volName, numBuckets);
+      int res;
+
+      out.reset();
+      res = ToolRunner.run(shell, new String[]{"-ls", "/" + volName});
+      assertEquals(0, res);
+      String r = out.toString(DEFAULT_ENCODING);
+      assertTrue(r.matches("(?s)^Found " + numBuckets + " items.*"));

Review Comment:
   nit: please use `assertThat` for better failure message
   
   ```suggestion
         assertThat(r).matches("(?s)^Found " + numBuckets + " items.*");
   ```



##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -882,9 +882,9 @@ public List<FileStatusAdapter> listStatus(String pathStr, 
boolean recursive,
     }
     OFSPath ofsStartPath = new OFSPath(startPath, config);
     if (ofsPath.isVolume()) {
-      String startBucket = ofsStartPath.getBucketName();
+      String startBucketPath = ofsStartPath.getNonKeyPath();
       return listStatusVolume(ofsPath.getVolumeName(),
-          recursive, startBucket, numEntries, uri, workingDir, username);
+          recursive, startBucketPath, numEntries, uri, workingDir, username);

Review Comment:
   I suggest changing `startPath` parameter of `listStatusVolume` to `OFSPath`, 
and passing `ofsStartPath`.  This reduces the chance for such misuse, and also 
avoids creating another instance of `OFSPath` for the same path in the method.  
The method may also validate that `ofsStartPath.getVolumeName()` equals 
`volumeNameStr`.



-- 
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]

Reply via email to