jojochuang commented on code in PR #10209:
URL: https://github.com/apache/ozone/pull/10209#discussion_r3584604006
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -865,6 +866,91 @@ public BlockLocation[] getFileBlockLocations(FileStatus
fileStatus,
}
}
+ @Override
+ public ContentSummary getContentSummary(Path f) throws IOException {
Review Comment:
**Low — maintenance risk:** `getContentSummary()` here largely duplicates
`BasicRootedOzoneFileSystem.getContentSummaryInSpan()`. Not a runtime bug
today, but future EC-policy changes may need to be applied in two places.
Consider extracting a shared helper if you touch this again.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java:
##########
@@ -403,6 +404,49 @@ public void testCreateKeyWithECReplicationConfig() throws
Exception {
createKeyWithECReplicationConfig(root, cluster.getConf());
}
+ @Test
+ void testContentSummaryErasureCodingPolicy() throws Exception {
+ String ratisKey = "ratis-ec-policy-key";
+ String ecKey = "ec-policy-key";
+ ECReplicationConfig ecConfig = new ECReplicationConfig("RS-3-2-1024k");
+ Path parentDir = new Path(OZONE_URI_DELIMITER, "ec-policy-mixed-o3fs");
+ Path ratisFile = new Path(parentDir, ratisKey);
+ Path ecFile = new Path(parentDir, ecKey);
+
+ fs.mkdirs(parentDir);
+ String ratisRelKey = "ec-policy-mixed-o3fs/" + ratisKey;
+ String ecRelKey = "ec-policy-mixed-o3fs/" + ecKey;
+ TestDataUtil.createKey(ozoneBucket, ratisRelKey,
+ RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE),
+ new byte[]{0});
+ TestDataUtil.createKey(ozoneBucket, ecRelKey, ecConfig,
+ new byte[]{0});
+
+ try {
+ assertEquals("",
+ fs.getContentSummary(ROOT).getErasureCodingPolicy());
+ assertEquals("Replicated",
+ fs.getContentSummary(ratisFile).getErasureCodingPolicy());
+ assertEquals(ecConfig.getReplication(),
+ fs.getContentSummary(ecFile).getErasureCodingPolicy());
+ assertEquals("",
+ fs.getContentSummary(parentDir).getErasureCodingPolicy());
+ } finally {
+ fs.delete(parentDir, true);
+ }
+ }
+
+ @Test
+ void testLsDashEDoesNotThrow() throws Exception {
Review Comment:
**Medium — weak regression coverage:** `testLsDashEDoesNotThrow` only checks
exit code `0`. It does not verify that `ls -e` prints the expected EC policy
values (`Replicated`, `RS-3-2-1024k`, etc.).
Consider capturing `FsShell` output (or asserting on `getContentSummary()`
in the same test setup) so a partial fix that still returns `null` on some
paths would be caught.
Same note applies to
`AbstractRootedOzoneFileSystemTest.testLsDashEDoesNotThrow` (~line 1902).
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -865,6 +866,91 @@ public BlockLocation[] getFileBlockLocations(FileStatus
fileStatus,
}
}
+ @Override
+ public ContentSummary getContentSummary(Path f) throws IOException {
+ Path qualifiedPath = f.makeQualified(uri, workingDir);
+ String key = pathToKey(qualifiedPath);
+ FileStatusAdapter status;
+ try {
+ status = adapter.getFileStatus(key, uri, qualifiedPath, getUsername());
+ } catch (OMException ex) {
+ if (ex.getResult().equals(OMException.ResultCodes.KEY_NOT_FOUND)) {
+ throw new FileNotFoundException("File not found. path:" + f);
+ }
+ throw ex;
+ }
+
+ if (status.isFile()) {
+ long length = status.getLength();
+ long spaceConsumed = status.getDiskConsumed();
+ ContentSummary.Builder builder = new
ContentSummary.Builder().length(length).
+ fileCount(1).directoryCount(0).spaceConsumed(spaceConsumed);
+ applyEcPolicy(builder, status.getErasureCodingPolicy());
+ return builder.build();
+ }
+
+ long[] summary = {0, 0, 0, 1};
+ for (FileStatusAdapter s : listStatusAdapter(f)) {
+ long length = s.getLength();
+ long spaceConsumed = s.getDiskConsumed();
+ ContentSummary c;
+ if (s.isDir()) {
+ c = getContentSummary(s.getPath());
+ } else {
+ ContentSummary.Builder childBuilder = new
ContentSummary.Builder().length(length).
+ fileCount(1).directoryCount(0).spaceConsumed(spaceConsumed);
+ applyEcPolicy(childBuilder, s.getErasureCodingPolicy());
+ c = childBuilder.build();
+ }
+
+ summary[0] += c.getLength();
+ summary[1] += c.getSpaceConsumed();
+ summary[2] += c.getFileCount();
+ summary[3] += c.getDirectoryCount();
+ }
+
+ ContentSummary.Builder builder = new
ContentSummary.Builder().length(summary[0]).
+ fileCount(summary[2]).directoryCount(summary[3]).
+ spaceConsumed(summary[1]);
+ applyEcPolicy(builder, status.getErasureCodingPolicy());
+ return builder.build();
+ }
+
+ /**
+ * Apply the erasure coding policy on the {@link ContentSummary.Builder}.
+ * Default implementation is a no-op so that this class can compile and run
+ * against Hadoop 2, where {@code ContentSummary.Builder.erasureCodingPolicy}
+ * does not exist. The Hadoop 3 subclass overrides this to set the policy.
+ */
+ protected void applyEcPolicy(ContentSummary.Builder builder, String
ecPolicy) {
Review Comment:
**Low — Hadoop 2 behavior:** `applyEcPolicy()` is intentionally a no-op here
so this compiles against Hadoop 2, where
`ContentSummary.Builder.erasureCodingPolicy()` does not exist. On Hadoop 2
builds, `getErasureCodingPolicy()` may still be `null` and `ls -e` can still
throw `UnsupportedOperationException`.
Fine if Hadoop 3-only support for `-e` is intended — worth a one-line
comment in the PR description or release note.
--
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]
