You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@ozone.apache.org by "kostacie (via GitHub)" <gi...@apache.org> on 2025/05/06 11:32:48 UTC

[PR] HDDS-11954. List keys command gets wrong info from BasicOmKeyInfo [ozone]

kostacie opened a new pull request, #8401:
URL: https://github.com/apache/ozone/pull/8401

   ## What changes were proposed in this pull request?
   In `OzoneBucket#toOzoneKey`, the parameter `isFile` for `OzoneKey` was changed because it showed the wrong value `true` for directories. 
   It was received from `BasicOmKeyInfo` which always was `true`, now it's taken from `OzoneFileStatusLight` which stores the correct value for `isFile`, if it is a directory.
   
   ## What is the link to the Apache JIRA
   [HDDS-11954](https://issues.apache.org/jira/browse/HDDS-11954)
   
   ## How was this patch tested?
   https://github.com/kostacie/ozone/actions/runs/14855232896
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

Re: [PR] HDDS-11954. List keys command gets wrong info from BasicOmKeyInfo [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.

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


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTestWithFSO.java:
##########
@@ -226,4 +231,31 @@ void testLeaseRecoverable() throws Exception {
     }
   }
 
+  @Test
+  void testIsFileFalseForDir() throws IOException {
+    Path dirPath = new Path(getBucketPath(), "dir1");
+    Path keyPath = new Path(dirPath, "key1");
+    OFSPath ofsPath = new OFSPath(dirPath, new OzoneConfiguration());
+
+    getFs().mkdirs(dirPath);
+    try (FSDataOutputStream out1 = getFs().create(keyPath)) {
+      out1.write(2);
+    }
+
+    RootedOzoneFileSystem ofs = (RootedOzoneFileSystem) getFs();
+    BasicRootedOzoneClientAdapterImpl adapter = (BasicRootedOzoneClientAdapterImpl) ofs.getAdapter();
+    OzoneBucket bucket = adapter.getBucket(ofsPath, false);
+
+    Iterator<? extends OzoneKey> key = bucket.listKeys("");

Review Comment:
   nit: rename to `keyIterator` or similar



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTestWithFSO.java:
##########
@@ -226,4 +231,31 @@ void testLeaseRecoverable() throws Exception {
     }
   }
 
+  @Test
+  void testIsFileFalseForDir() throws IOException {
+    Path dirPath = new Path(getBucketPath(), "dir1");
+    Path keyPath = new Path(dirPath, "key1");
+    OFSPath ofsPath = new OFSPath(dirPath, new OzoneConfiguration());

Review Comment:
   Can use existing `conf` member after moving to parent class.
   
   ```suggestion
       OFSPath ofsPath = new OFSPath(dirPath, conf);
   ```



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTestWithFSO.java:
##########
@@ -226,4 +231,31 @@ void testLeaseRecoverable() throws Exception {
     }
   }
 
+  @Test
+  void testIsFileFalseForDir() throws IOException {
+    Path dirPath = new Path(getBucketPath(), "dir1");
+    Path keyPath = new Path(dirPath, "key1");
+    OFSPath ofsPath = new OFSPath(dirPath, new OzoneConfiguration());
+
+    getFs().mkdirs(dirPath);
+    try (FSDataOutputStream out1 = getFs().create(keyPath)) {
+      out1.write(2);
+    }
+
+    RootedOzoneFileSystem ofs = (RootedOzoneFileSystem) getFs();
+    BasicRootedOzoneClientAdapterImpl adapter = (BasicRootedOzoneClientAdapterImpl) ofs.getAdapter();

Review Comment:
   Please move this test case to the parent class `AbstractRootedOzoneFileSystemTest`.  Then these local variables `ofs` and `adapter` are unnecessary.



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTestWithFSO.java:
##########
@@ -226,4 +231,31 @@ void testLeaseRecoverable() throws Exception {
     }
   }
 
+  @Test
+  void testIsFileFalseForDir() throws IOException {
+    Path dirPath = new Path(getBucketPath(), "dir1");
+    Path keyPath = new Path(dirPath, "key1");
+    OFSPath ofsPath = new OFSPath(dirPath, new OzoneConfiguration());
+
+    getFs().mkdirs(dirPath);
+    try (FSDataOutputStream out1 = getFs().create(keyPath)) {
+      out1.write(2);
+    }
+
+    RootedOzoneFileSystem ofs = (RootedOzoneFileSystem) getFs();
+    BasicRootedOzoneClientAdapterImpl adapter = (BasicRootedOzoneClientAdapterImpl) ofs.getAdapter();
+    OzoneBucket bucket = adapter.getBucket(ofsPath, false);
+
+    Iterator<? extends OzoneKey> key = bucket.listKeys("");
+
+    assertTrue(key.hasNext(), "Expected dir1, key1 in the bucket");
+    OzoneKey ozoneKey = key.next();
+    assertEquals("dir1/", ozoneKey.getName());
+    assertFalse(ozoneKey.isFile(), "Expected isFile to be false for directory key");
+    ozoneKey = key.next();
+    assertEquals("dir1/key1", ozoneKey.getName());
+    assertTrue(ozoneKey.isFile(), "Expected isFile to be true for key");
+
+    getFs().delete(dirPath, true);

Review Comment:
   Cleanup is done in `@AfterEach`, I don't think it's needed here.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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

Re: [PR] HDDS-11954. List keys command gets wrong info from BasicOmKeyInfo [ozone]

Posted by "kostacie (via GitHub)" <gi...@apache.org>.

kostacie commented on code in PR #8401:
URL: https://github.com/apache/ozone/pull/8401#discussion_r2083941656


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java:
##########
@@ -2588,4 +2589,27 @@ private void deleteFiles(Path base, List<String> fileNames) throws IOException {
     }
   }
 
+  @Test
+  void testIsFileFalseForDir() throws IOException {

Review Comment:
   Done.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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