HDDS-13305. Create wrapper object for container checksums #8789

echonesis · 2025-07-11T09:27:23Z

What changes were proposed in this pull request?

As mentioned in the JIRA, this PR will

create ContainerChecksums Wrapper to include dataChecksum and metadataChecksum
provide further extension by metadataChecksum

What is the link to the Apache JIRA

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

How was this patch tested?

CI: https://github.com/echonesis/ozone/actions/runs/16518174299

adoroszlai

Thanks @echonesis for the patch.

...ds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/ContainerChecksumsTest.java

adoroszlai · 2025-07-22T07:40:28Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+  public ContainerChecksums(long dataChecksum) {
+    this(dataChecksum, null);
+  }
+
+  public ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+    this.dataChecksum = dataChecksum;
+    this.metadataChecksum = metadataChecksum;
+  }


We could add factory methods (and make constructor private) to reduce accidental mismatch of intended and actual source (data or metadata).

Something like:

public static ContainerChecksums unknown(); // use constant value public static ContainerChecksums dataOnly(long dataChecksum); public static ContainerChecksums metadataOnly(long metadataChecksum); public static ContainerChecksums of(long dataChecksum, long metadataChecksum);

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

adoroszlai · 2025-07-22T07:44:14Z

...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java

    // All replicas should start with an empty data checksum in SCM.
    boolean contOneDataChecksumsEmpty = containerManager.getContainerReplicas(contID).stream()
-        .allMatch(r -> r.getDataChecksum() == 0);
+        .allMatch(r -> r.getChecksums().getDataChecksum() == 0);


We could reduce change by keeping the getDataChecksum() method in ContainerReplica and delegate to checksums.

adoroszlai · 2025-07-23T06:06:21Z

Thanks @echonesis for updating the patch. Just one more question: why does ContainerChecksums use long 0 for unknown data checksum, and Long null for unknown/missing metadata checksum? Is there a use case where there is a difference between 0 and null?

echonesis · 2025-07-23T06:31:56Z

Thanks @echonesis for updating the patch. Just one more question: why does ContainerChecksums use long 0 for unknown data checksum, and Long null for unknown/missing metadata checksum? Is there a use case where there is a difference between 0 and null?

I believe metadata serves a critical role in future operations and analysis. Regarding the toString() method, I propose we differentiate between two checksum types: the existing data checksum and a new metadata checksum.
I'm treating this as a schema extension, specifically by introducing nullability. This ensures backward compatibility, so existing consumers using older APIs won't experience breakage.

adoroszlai

Thanks @echonesis for updating the patch.

errose28

Thanks for adding this @echonesis. Mostly looks good from the SCM and Recon side, just a few comments on top of what has already been left. We will need to add this to the datanode as well within ContainerData, ContainerLogger, and the summary section of KeyValueHandler#reconcileContainer, but it is probably better to do the datanode side in a follow-up PR.

errose28 · 2025-07-23T21:18:01Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+    return new ContainerChecksums(dataChecksum, null);
+  }
+
+  public static ContainerChecksums metadataOnly(long metadataChecksum) {


We will have metadata checksum in the future, but since we do't have it right now, we don't need to include it in this change, as long as it is easy to add new checksums to this class in the future.

Currently we only need two types of checksum objects: Those that have no checksum and those that have a data checksum. When metadata checksum is added in the future for EC, it will be generated by the container scanner and set at the same time as the data checksum, so it will not be possible to have a ContainerChecksums object with only one checksum or the other. We can adjust the factory constructors accordingly.

Thanks @errose28
I will adjust ContainerChecksums constructors to be

UNKNOWN (no checksums)

2 checksums included (dataChecksum + metadataChecksum)

errose28 · 2025-07-23T21:23:41Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+      new ContainerChecksums(0, null);
+
+  private final long dataChecksum;
+  private final Long metadataChecksum; // nullable for future use


See the recently merged #8565 for how we are planning to handle unset checksums. We can add this handling into the ContainerChecksums class so that the -1 placeholder gives us functionality similar to the has checks in protobuf objects.

errose28 · 2025-07-23T21:30:45Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+    if (metadataChecksum != null) {
+      sb.append(", metadata=").append(Long.toHexString(metadataChecksum));
+    }


0 should be the displayed value for any checksum that is not set.

Got it. I will set the default metadataChecksum value to be 0.

errose28 · 2025-07-23T21:36:20Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplica.java

+    return checksums;
+  }
+
  public long getDataChecksum() {


I think we can just use getChecksums in place of getters for each individual checksum. Otherwise this class will need to be updated every time ContainerChecksums has a value added to it.

long getDataChecksum was retained on my request to reduce change in existing tests. We don't need to adjust this class for future values.

errose28 · 2025-07-23T21:37:51Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.container;


We need this class on the datanode too, so it needs to be in a more general package.

Got it.
I will move it into the path under hadoop-hdds/common.

errose28 · 2025-07-23T22:00:32Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java

  public boolean isDataChecksumMismatched() {
    return !replicas.isEmpty() && replicas.stream()
-            .map(ContainerReplica::getDataChecksum)
+            .map(ContainerReplica::getChecksums)
+            .map(ContainerChecksums::getDataChecksum)
            .distinct()
            .count() != 1;


This page in recon is designed to show containers with any checksum mismatches. We can rename the method accordingly and simplify the check so that it uses ContainerChecksum#equals to automatically compare all checksums going forward.

Suggested change

public boolean isDataChecksumMismatched() {

return !replicas.isEmpty() && replicas.stream()

.map(ContainerReplica::getDataChecksum)

.map(ContainerReplica::getChecksums)

.map(ContainerChecksums::getDataChecksum)

.distinct()

.count() != 1;

public boolean areChecksumsMismatched() {

return !replicas.isEmpty() && replicas.stream()

.map(ContainerReplica::getChecksums)

.distinct()

.count() != 1;

errose28 · 2025-07-23T22:24:54Z

...t/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java


    waitForScmToSeeReplicaState(containerID, CLOSED);
-    long initialReportedDataChecksum = getContainerReplica(containerID).getDataChecksum();
+    long initialReportedDataChecksum = getContainerReplica(containerID).getChecksums().getDataChecksum();


For this use case we only need to use the ContainerChecksums objects and compare them. We don't need to further extract the long values.

There is a new assertReplicaChecksumMatches introduced in #8565 that can take the ContainerChecksums object as a parameter instead of a long.

Got it.
I've updated assertReplicaChecksumMatches by replacing long with ContainerChecksums.

adoroszlai · 2025-07-30T19:19:27Z

@errose28 please take another look

github-actions · 2025-11-11T00:06:02Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions · 2025-11-25T00:05:16Z

Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.

adoroszlai

Thanks @echonesis for working on this and sorry for the late review.

adoroszlai · 2025-12-15T12:42:45Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+public final class ContainerChecksums {
+
+  private static final ContainerChecksums UNKNOWN =
+      new ContainerChecksums(-1L, -1L);


nit: use UNSET_DATA_CHECKSUM and UNSET_METADATA_CHECKSUM

adoroszlai · 2025-12-15T12:43:52Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+  public long getDataChecksum() {
+    // UNSET_DATA_CHECKSUM is an internal placeholder, it should not be used outside this class.
+    if (needsDataChecksum()) {


nit: move comment to UNSET_DATA_CHECKSUM definition

adoroszlai · 2025-12-15T12:44:35Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+  public long getMetadataChecksum() {
+    // UNSET_DATA_CHECKSUM is an internal placeholder, it should not be used outside this class.
+    if (needsMetadataChecksum()) {


nit: this should mention UNSET_METADATA_CHECKSUM, also please move comment to UNSET_METADATA_CHECKSUM definition

adoroszlai · 2025-12-15T13:19:58Z

...r-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java

        .setBytesUsed(replicaProto.getUsed())
        .setEmpty(replicaProto.getIsEmpty())
-        .setDataChecksum(replicaProto.getDataChecksum())
+        .setChecksums(ContainerChecksums.of(replicaProto.getDataChecksum(), 0L))


Please replace all calls like this with ContainerChecksums.dataOnly(...) as suggested earlier.

Rationale:

Avoid duplication of 0L as unknown value

Avoid mismatch between internal-only constant with value -1L

Easier to understand

Thanks @adoroszlai
According to the review from @errose28 , it seems that we won't have a ContainerChecksums with only one checksum or the other.
And yes, I will follow the rationale.

Currently we only have data checksum, so we should not force all callers to pass 0L for metadata. I agree that we don't need metadataOnly(), but the factory method with single data checksum value should still be added. To reduce future changes (adding metadata checksum value), its name should be of() rather than dataOnly().

Thanks for pointing it out!
I will modify it in the next commit.

adoroszlai · 2025-12-15T13:31:06Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java

+    return getDataChecksum() == that.getDataChecksum() &&
+        getMetadataChecksum() == that.getMetadataChecksum();
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(dataChecksum, metadataChecksum);


ContainerChecksums(0, X) and ContainerChecksums(-1, X) are equal, but hash codes are different, which is a bad combination.

Do we want to consider these unequal? Then equals should use the variables.

Otherwise, if we want to consider these to be equal, and all other code passes 0 for unknown value, can we avoid using -1 internally? That would also make needs... methods unnecessary.

I agree that we should consider this unequality.
In #8565, it seems the UNSET placeholder could help us separate the 2 states:

UNSET state

Calculated state

adoroszlai · 2025-12-15T13:36:03Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java

    this.bcsId = bcsId;
    this.state = state;
-    this.dataChecksum = dataChecksum;
+    this.checksums = checksums;


Suggested change

this.checksums = checksums;

setChecksums(checksums);

adoroszlai · 2025-12-15T13:36:11Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java

+  }
+
+  public void setChecksums(ContainerChecksums checksums) {
+    this.checksums = checksums;


Suggested change

this.checksums = checksums;

this.checksums = checksums != null ? checksums : ContainerChecksums.unknown();

adoroszlai · 2025-12-15T13:38:33Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java

        .setFirstSeenTime(firstSeenTime).setLastSeenTime(lastSeenTime)
-        .setBcsId(bcsId).setState(state).setDataChecksum(dataChecksum).build();
+        .setBcsId(bcsId).setState(state)
+        .setDataChecksum(checksums != null ? checksums.getDataChecksum() : 0L)


Suggested change

.setDataChecksum(checksums != null ? checksums.getDataChecksum() : 0L)

.setDataChecksum(checksums.getDataChecksum())

echonesis · 2025-12-16T14:48:37Z

Thanks @adoroszlai for the review.
I will update them in the next commit.

adoroszlai

Thanks @echonesis for updating the patch, LGTM. There is one caller where I think 0L should be removed. Tests that set arbitrary checksum value are OK to use 0L.

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java

adoroszlai · 2025-12-18T08:41:54Z

@errose28 please take another look

adoroszlai · 2025-12-19T11:25:27Z

Thanks @echonesis for the patch, @errose28 for the review.

HDDS-13305. Create wrapper object for container checksums

8f59ac0

echonesis marked this pull request as draft July 11, 2025 09:28

echonesis marked this pull request as ready for review July 12, 2025 01:40

Merge branch 'master' into HDDS-13305

3323de2

ivandika3 requested review from aswinshakil and errose28 July 15, 2025 05:38

errose28 added the container-reconciliation label Jul 15, 2025

merge fix

902997d

adoroszlai reviewed Jul 22, 2025

View reviewed changes

echonesis added 3 commits July 23, 2025 11:18

PR review modification update

1299510

checkstyle update

f0e922f

Replica modification update

0253d2d

echonesis requested a review from adoroszlai July 23, 2025 08:17

adoroszlai approved these changes Jul 23, 2025

View reviewed changes

errose28 requested changes Jul 23, 2025

View reviewed changes

errose28 reviewed Jul 23, 2025

View reviewed changes

echonesis added 5 commits July 24, 2025 10:09

merge fix

ed449d4

review update

c9425f4

placeholder update

242f2e4

checkstyle update

cffc5c2

conflict fix

620efb0

echonesis requested a review from errose28 July 25, 2025 07:54

echonesis added 2 commits July 25, 2025 16:38

reduce mod test

42d61cf

ra test

5fd256e

github-actions bot added the stale label Nov 11, 2025

github-actions bot closed this Nov 25, 2025

adoroszlai reopened this Dec 15, 2025

adoroszlai reviewed Dec 15, 2025

View reviewed changes

adoroszlai removed the stale label Dec 15, 2025

echonesis added 3 commits December 17, 2025 01:15

fix: CR update

f4ef042

fix: CR update

df89c7f

Merge branch 'master' into HDDS-13305-dev

d5cf9d9

adoroszlai reviewed Dec 18, 2025

View reviewed changes

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java Outdated Show resolved Hide resolved

Use data-only factory method in ContainerReplicaHistory

bb8bb9f

adoroszlai merged commit b32b54b into apache:master Dec 19, 2025
83 of 84 checks passed

echonesis deleted the HDDS-13305 branch December 24, 2025 02:59

echonesis added a commit to echonesis/ozone that referenced this pull request Dec 24, 2025

HDDS-13305. Create wrapper object for container checksums (apache#8789)

559037c

	this.checksums = checksums;
	this.checksums = checksums != null ? checksums : ContainerChecksums.unknown();

	.setDataChecksum(checksums != null ? checksums.getDataChecksum() : 0L)
	.setDataChecksum(checksums.getDataChecksum())

HDDS-13305. Create wrapper object for container checksums #8789

HDDS-13305. Create wrapper object for container checksums #8789

Uh oh!

Conversation

echonesis commented Jul 11, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jul 23, 2025

Uh oh!

echonesis commented Jul 23, 2025

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

errose28 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!

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!

echonesis Jul 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!

adoroszlai commented Jul 30, 2025

Uh oh!

github-actions bot commented Nov 11, 2025

Uh oh!

github-actions bot commented Nov 25, 2025

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!

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

echonesis commented Jul 11, 2025 •

edited

Loading

echonesis Jul 24, 2025 •

edited

Loading

echonesis Dec 17, 2025 •

edited

Loading