HDDS-6611. Remove chunksPath and metadataPath from container yaml file #7930

ChenSammi · 2025-02-19T10:08:29Z

What changes were proposed in this pull request?

Remove chunkPath and metadataPath from container yaml file

Changes made in this patch,

add a DATANODE_SCHEMA_V4 Datanode Layout feature
add a "4" schemaVersion for container schema V4
behavior
1. before feature "DATANODE_SCHEMA_V4" is finalized, new createContainerRequest/importContainer will create V3 container
2. after feature "DATANODE_SCHEMA_V4" is finalized, createContainerRequest will create V4 container. If import a schema V3 container, it will be automatically converted to V4 schema container. When a V3 container has chance to update its yaml file, it will be converted to V4 automatically too. A schema V2 container will not change, remains as a V2 container.

So if it's a V2/V3 schema container, its yaml file will have chunkPath and metadataPath. If it's a V4 schema container, then its yaml file doesn't have chunkPath and metadataPath.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6611

How was this patch tested?

new unit tests
enabled Schema V4 for some existing unit tests which test container

adoroszlai

Thanks @ChenSammi for the patch. Mostly LGTM, few minor improvements suggested.

adoroszlai · 2025-02-20T08:40:12Z

...-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerData.java

+    if (isFinalized(HDDSLayoutFeature.DATANODE_SCHEMA_V4)) {
+      return KV_YAML_FIELDS_SCHEMA_V4;
+    } else {
+      return Collections.unmodifiableList(KV_YAML_FIELDS);
+    }


It should return unmodifiableList in both cases.

Suggested change

if (isFinalized(HDDSLayoutFeature.DATANODE_SCHEMA_V4)) {

return KV_YAML_FIELDS_SCHEMA_V4;

} else {

return Collections.unmodifiableList(KV_YAML_FIELDS);

}

List<String> list = isFinalized(HDDSLayoutFeature.DATANODE_SCHEMA_V4)

? KV_YAML_FIELDS_SCHEMA_V4

: KV_YAML_FIELDS;

return Collections.unmodifiableList(list);

adoroszlai · 2025-02-20T08:47:24Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataYaml.java

+        if (!kvData.hasSchema(SCHEMA_V4)) {
+          kvData.setMetadataPath((String) nodes.get(OzoneConsts.METADATA_PATH));
+          kvData.setChunksPath((String) nodes.get(OzoneConsts.CHUNKS_PATH));


This condition would need to be updated if SCHEMA_V5 is introduced. Can it be changed to something like: kvData.hasSchemaBefore(SCHEMA_V4)? Please feel free to come up with a better name.

(Same applies to conversion to YAML.)

adoroszlai · 2025-02-20T08:58:33Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    conf.setBoolean(OzoneConfigKeys.HDDS_CONTAINER_RATIS_DATASTREAM_ENABLED, true);
+    conf.setBoolean(OzoneConfigKeys.HDDS_CONTAINER_RATIS_DATASTREAM_RANDOM_PORT, true);


Is datastream used in the test? Is it needed?

adoroszlai · 2025-02-20T09:03:37Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    // sleep 1s to make sure creationTime will change
+    Thread.sleep(1000);


Can we avoid this? Not only does it make tests slower, assertions about timing are flaky when relying on sleep.

If prod code already uses Clock, we can use TestClock in the tests. Otherwise, I'd propose removing assertions about creationTime.

The sleep here is for detecting the container directory is recreated during container import. Without this sleep, the creationTime of old directory and new directory most time will be same. I prefer to keep the creationTime here. As the creationTime is set by OS file system, I guess TestClock is not applicable here.

adoroszlai · 2025-02-20T09:03:57Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    assertTrue(new File(newContainerData.getContainerPath()).exists());
+    assertTrue(new File(newContainerData.getChunksPath()).exists());
+    assertTrue(new File(newContainerData.getMetadataPath()).exists());
+    if (schemaV3Enabled) {
+      assertTrue(newContainerData.getDbFile().exists());


nit: assertThat(<some File>).exists(); provides better error message if the assertion fails.

adoroszlai · 2025-02-20T09:04:53Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    assertEquals(newContainerData.getContainerDBType(), oldContainerData.getContainerDBType());
+    assertEquals(newContainerData.getState(), oldContainerData.getState());
+    assertEquals(newContainerData.getBlockCount(), oldContainerData.getBlockCount());
+    assertEquals(newContainerData.getLayoutVersion(), oldContainerData.getLayoutVersion());
+    assertEquals(newContainerData.getMaxSize(), oldContainerData.getMaxSize());
+    assertEquals(newContainerData.getBytesUsed(), oldContainerData.getBytesUsed());
+    assertEquals(newContainerData.getMetadataPath(), oldContainerData.getMetadataPath());
+    assertEquals(newContainerData.getChunksPath(), oldContainerData.getChunksPath());
+    assertEquals(newContainerData.getContainerPath(), oldContainerData.getContainerPath());


nit: Please extract a method to avoid duplicating a bunch of assertions in test cases.

errose28 · 2025-02-20T19:00:57Z

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java

+  public static final String CONTAINER_SCHEMA_V4_ENABLED =
+      "hdds.datanode.container.schema.v4.enabled";
+  public static final boolean CONTAINER_SCHEMA_V4_ENABLED_DEFAULT = true;


We had issues with the schema v3 feature flag in the past because if it was enabled then disabled, the schema v3 containers would not get loaded. I don't think we should have config keys for container schema versions.

It's merely for some existing test cases specifically for V3. Let me check if they can be refactored.

ChenSammi · 2025-02-25T07:56:36Z

@adoroszlai @errose28 , thanks for the review. Comments are addressed. Would you like to take another look?

adoroszlai · 2025-02-25T08:08:04Z

Thanks @ChenSammi for updating the patch. Let's wait for @errose28 to take a look.

ivanzlenko · 2025-02-25T11:05:50Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/upgrade/HDDSLayoutFeature.java

  HBASE_SUPPORT(8, "Datanode RocksDB Schema Version 3 has an extra table " +
-          "for the last chunk of blocks to support HBase.)");
+          "for the last chunk of blocks to support HBase.)"),
+  DATANODE_SCHEMA_V4(9, "Container yaml file doesn't require chunksPath and metadataPath");


Suggested change

DATANODE_SCHEMA_V4(9, "Container yaml file doesn't require chunksPath and metadataPath");

DATANODE_SCHEMA_V4(9, "Container YAML file doesn't require chunksPath and metadataPath");

ivanzlenko · 2025-02-25T11:10:31Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java

+  // V4: Column families is same as V3,
+  // removed chunkPath and metadataPath in .container file


Suggested change

// V4: Column families is same as V3,

// removed chunkPath and metadataPath in .container file

/**

* Schema version 4 for Ozone container files.

* <p>

* The column families remain the same as defined in {@link #SCHEMA_V3}.

* However, the {@code chunkPath} and {@code metadataPath}

* fields have been removed in this version of the .container files.

* </p>

*/

ivanzlenko · 2025-02-25T11:13:01Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java


      Yaml yaml = ContainerDataYaml.getYamlForContainerType(
-          containerData.getContainerType(),
+          containerData.getContainerType(), containerData,


Suggested change

containerData.getContainerType(), containerData,

containerData.getContainerType(),

containerData,

ivanzlenko · 2025-02-25T11:14:36Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataYaml.java

-      return readContainer(inputFileStream);
+      KeyValueContainerData containerData = (KeyValueContainerData) readContainer(inputFileStream);
+      if (containerData.getChunksPath() == null) {
+        containerData.setChunksPath(containerFile.getParentFile().getParentFile().getAbsolutePath()


Please move containerFile.getParentFile().getParentFile().getAbsolutePath() .concat(OZONE_URI_DELIMITER).concat(STORAGE_DIR_CHUNKS) into separate variable.

ivanzlenko · 2025-02-25T11:18:05Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataYaml.java

        yamlFields = new ArrayList<>(yamlFields);
        yamlFields.add(REPLICA_INDEX);
      }
+      if (((KeyValueContainerData)containerData).olderSchemaThan(SCHEMA_V4)) {


Suggested change

if (((KeyValueContainerData)containerData).olderSchemaThan(SCHEMA_V4)) {

if (((KeyValueContainerData) containerData).olderSchemaThan(SCHEMA_V4)) {

ivanzlenko · 2025-02-25T13:28:40Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    KeyValueContainerData newContainerData =
+        new KeyValueContainerData(containerID1,
+            oldContainerData.getLayoutVersion(),
+            oldContainerData.getMaxSize(), pipeline.getId().getId().toString(),


Suggested change

oldContainerData.getMaxSize(), pipeline.getId().getId().toString(),

oldContainerData.getMaxSize(),

pipeline.getId().getId().toString(),

ivanzlenko · 2025-02-25T13:29:33Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+      KeyValueContainerData data = (KeyValueContainerData) ContainerDataYaml
+          .readContainer(containerDescriptorYaml);


Suggested change

KeyValueContainerData data = (KeyValueContainerData) ContainerDataYaml

.readContainer(containerDescriptorYaml);

KeyValueContainerData data = (KeyValueContainerData) ContainerDataYaml.readContainer(containerDescriptorYaml);

ivanzlenko · 2025-02-25T13:30:01Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+      newContainer.importContainerData(fis, packer);
+    }
+
+    assertTrue(isContainerEqual(newContainerData, oldContainerData));


Please use assertThat

ivanzlenko · 2025-02-25T13:30:54Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+        newContainer.getContainerData().getChunksPath());
+    assertEquals(yamlFile.getParentFile().getAbsolutePath(), newContainer.getContainerData().getMetadataPath());
+    FileTime creationTime2 = (FileTime) Files.getAttribute(
+        Paths.get(newContainer.getContainerData().getContainerPath()), "creationTime");


Please move into a separate variable

ivanzlenko · 2025-02-25T13:31:38Z

...e/src/test/java/org/apache/hadoop/ozone/container/upgrade/TestDatanodeUpgradeToSchemaV4.java

+    assertNotEquals(creationTime1.toInstant(), creationTime2.toInstant());
+  }
+
+  private boolean isContainerEqual(KeyValueContainerData containerData1, KeyValueContainerData containerData2) {


We do not need such method. Usually matchers have field-by-field comparison methods.

errose28 · 2025-02-26T04:42:44Z

@ChenSammi I've reviewed the code in context with your comment on the jira and I still do not think this requires a schema version. Actually the change should be much simpler without it:

For new containers, don't write the values to the yaml
For old containers, ignore the existing values except during hash calculation, where we will load all yaml fields.

Checking usages of OzoneConsts#SCHEMA_v4 we see most uses are boilerplate for adding a new schema in general. The places making decisions based on schema version are:

container import
- I don't see any advantage to migrating old containers on import. If we are finalized we can simply ignore the fields if they are present.
container file rewrite
- Same as above, I do not think we should be doing migration here.
Yaml hash calculation
- We can check if the path fields are present to make the same decisions without needing to check a schema version. This is already done in the current patch in ContainerDataYaml#readContainerFile.

ChenSammi · 2025-02-26T06:27:12Z

  * We can check if the path fields are present to make the same decisions without needing to check a schema version. This is already done in the current patch in `ContainerDataYaml#readContainerFile`.

@errose28 , it's different, you can check ContainerUtils#verifyChecksum, where the container yaml file checksum is verified. I would love to adopt it if it can verify both container yaml file with these two fields and without the two fields in an elegant way. One possible solution is read the entire container file into String first, check whether the String contains "chunksPath" and "metadataPath", and then create Yaml object.

ChenSammi · 2025-02-26T06:38:38Z

@ivanzlenko , thank you for the review. I will try to address some of them in next patch. For the rest, I would like to keep them, for example, keeping multiple parameters in one line. Actually one line one parameter is not a recommended style in Ozone, we should avoid to use that. And those style related comments in test case codes.

ivanzlenko · 2025-04-02T16:12:48Z

Actually one line one parameter is not a recommended style in Ozone, we should avoid to use that. And those style related comments in test case codes.

In many cases it is very much hurts readability:

callFunc(param1, param2, param3,
    param4, param5,
    aVeryLongParameterNameWhichTakesAlmostAllSpace,
    param7, param8);

Compared to:

callFunc(
    param1,
    param2,
    param3,
    param4,
    param5,
    aVeryLongParameterNameWhichTakesAlmostAllSpace,
    param7,
    param8
);

It is way easier to understand what is going on and which parameters are involved in a function call.
However it maybe worth pull into GitHub discussion.

adoroszlai · 2025-04-18T18:54:24Z

/pending conflicts, reviews

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

conflicts, reviews

github-actions · 2025-05-10T00:22:08Z

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

ChenSammi marked this pull request as draft February 19, 2025 10:09

HDDS-6611. Remove chunksPath and metadataPath from container yaml file

08d012b

ChenSammi force-pushed the HDDS-6611 branch from 88a04f0 to 08d012b Compare February 19, 2025 10:31

ChenSammi marked this pull request as ready for review February 19, 2025 10:32

fix import order

22932f7

adoroszlai reviewed Feb 20, 2025

View reviewed changes

errose28 reviewed Feb 24, 2025

View reviewed changes

ChenSammi added 3 commits February 24, 2025 16:27

address comments

8edb068

fix checkstyle

25ff254

fix failed UT

55720bd

ChenSammi force-pushed the HDDS-6611 branch from 197721d to 55720bd Compare February 24, 2025 13:37

remove System.out.println(content) which is for debug

b3c584d

adoroszlai mentioned this pull request Feb 25, 2025

HDDS-12413. Move field declarations to start of class in hdds-container-service #7968

Merged

ivanzlenko suggested changes Feb 25, 2025

View reviewed changes

github-actions bot requested changes Apr 18, 2025

View reviewed changes

github-actions bot added the pending label Apr 18, 2025

github-actions bot closed this May 10, 2025

ChenSammi mentioned this pull request May 16, 2025

HDDS-13023. Container checksum is missing after container import #8459

Merged

		conf.setBoolean(OzoneConfigKeys.HDDS_CONTAINER_RATIS_DATASTREAM_ENABLED, true);
		conf.setBoolean(OzoneConfigKeys.HDDS_CONTAINER_RATIS_DATASTREAM_RANDOM_PORT, true);

		// sleep 1s to make sure creationTime will change
		Thread.sleep(1000);

	DATANODE_SCHEMA_V4(9, "Container yaml file doesn't require chunksPath and metadataPath");
	DATANODE_SCHEMA_V4(9, "Container YAML file doesn't require chunksPath and metadataPath");

		// V4: Column families is same as V3,
		// removed chunkPath and metadataPath in .container file

-  // V4: Column families is same as V3,
-  // removed chunkPath and metadataPath in .container file
+  /**
+   * Schema version 4 for Ozone container files.
+   * <p>
+   * The column families remain the same as defined in {@link #SCHEMA_V3}.
+   * However, the {@code chunkPath} and {@code metadataPath}
+   * fields have been removed in this version of the .container files.
+   * </p>
+   */

	containerData.getContainerType(), containerData,
	containerData.getContainerType(),
	containerData,

	if (((KeyValueContainerData)containerData).olderSchemaThan(SCHEMA_V4)) {
	if (((KeyValueContainerData) containerData).olderSchemaThan(SCHEMA_V4)) {

	oldContainerData.getMaxSize(), pipeline.getId().getId().toString(),
	oldContainerData.getMaxSize(),
	pipeline.getId().getId().toString(),

		KeyValueContainerData data = (KeyValueContainerData) ContainerDataYaml
		.readContainer(containerDescriptorYaml);

HDDS-6611. Remove chunksPath and metadataPath from container yaml file #7930

HDDS-6611. Remove chunksPath and metadataPath from container yaml file #7930

Uh oh!

Conversation

ChenSammi commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Feb 25, 2025

Uh oh!

adoroszlai commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errose28 commented Feb 26, 2025

Uh oh!

ChenSammi commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChenSammi commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanzlenko commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai commented Apr 18, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ChenSammi commented Feb 19, 2025 •

edited

Loading

ChenSammi Feb 24, 2025 •

edited

Loading

ChenSammi commented Feb 26, 2025 •

edited

Loading

ChenSammi commented Feb 26, 2025 •

edited

Loading

ivanzlenko commented Apr 2, 2025 •

edited

Loading