Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.hdds.scm.container;

import java.util.Objects;
import net.jcip.annotations.Immutable;

/**
* Wrapper for container checksums (data, metadata, etc.).
* Provides equality, hash, and hex string rendering.
*/
@Immutable
public final class ContainerChecksums {

private static final ContainerChecksums UNKNOWN =
new ContainerChecksums(-1L, -1L);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use UNSET_DATA_CHECKSUM and UNSET_METADATA_CHECKSUM


// Checksum of the data within the wrapper.
private final long dataChecksum;
private static final long UNSET_DATA_CHECKSUM = -1;

// Checksum of the metadata within the wrapper.
private final long metadataChecksum;
private static final long UNSET_METADATA_CHECKSUM = -1;

private ContainerChecksums(long dataChecksum, long metadataChecksum) {
this.dataChecksum = dataChecksum;
this.metadataChecksum = metadataChecksum;
}

public static ContainerChecksums unknown() {
return UNKNOWN;
}

public static ContainerChecksums of(long dataChecksum, long metadataChecksum) {
return new ContainerChecksums(dataChecksum, metadataChecksum);
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move comment to UNSET_DATA_CHECKSUM definition

return 0;
}
return dataChecksum;
}

public boolean needsDataChecksum() {
return dataChecksum == UNSET_DATA_CHECKSUM;
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 0;
}
return metadataChecksum;
}

public boolean needsMetadataChecksum() {
return metadataChecksum == UNSET_METADATA_CHECKSUM;
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (!(obj instanceof ContainerChecksums)) {
return false;
}
ContainerChecksums that = (ContainerChecksums) obj;
return getDataChecksum() == that.getDataChecksum() &&
getMetadataChecksum() == that.getMetadataChecksum();
}

@Override
public int hashCode() {
return Objects.hash(dataChecksum, metadataChecksum);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@echonesis echonesis Dec 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Override
public String toString() {
return "data=" + Long.toHexString(getDataChecksum()) +
", metadata=" + Long.toHexString(getMetadataChecksum());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.hdds.scm.container;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;

import org.junit.jupiter.api.Test;

class TestContainerChecksums {
@Test
void testEqualsAndHashCode() {
ContainerChecksums c1 = ContainerChecksums.of(123L, 0L);
ContainerChecksums c2 = ContainerChecksums.of(123L, 0L);
ContainerChecksums c3 = ContainerChecksums.of(456L, 0L);
ContainerChecksums c4 = ContainerChecksums.of(123L, 789L);
ContainerChecksums c5 = ContainerChecksums.of(123L, 789L);
ContainerChecksums c6 = ContainerChecksums.of(123L, 790L);

assertEquals(c1, c2);
assertEquals(c1.hashCode(), c2.hashCode());
assertNotEquals(c1, c3);
assertNotEquals(c1, c4);
assertEquals(c4, c5);
assertNotEquals(c4, c6);
}

@Test
void testToString() {
ContainerChecksums c1 = ContainerChecksums.of(0x1234ABCDL, 0L);
assertThat(c1.toString()).contains("data=1234abcd", "metadata=0");

ContainerChecksums c2 = ContainerChecksums.of(0x1234ABCDL, 0xDEADBEEFL);
assertThat(c2.toString()).contains("data=1234abcd").contains("metadata=deadbeef");

ContainerChecksums c3 = ContainerChecksums.unknown();
assertThat(c3.toString()).contains("data=0").contains("metadata=0");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ private void updateContainerReplica(final DatanodeDetails datanodeDetails,
.setReplicaIndex(replicaProto.getReplicaIndex())
.setBytesUsed(replicaProto.getUsed())
.setEmpty(replicaProto.getIsEmpty())
.setDataChecksum(replicaProto.getDataChecksum())
.setChecksums(ContainerChecksums.of(replicaProto.getDataChecksum(), 0L))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.build();

if (replica.getState().equals(State.DELETED)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public final class ContainerReplica implements Comparable<ContainerReplica> {
private final long keyCount;
private final long bytesUsed;
private final boolean isEmpty;
private final long dataChecksum;
private final ContainerChecksums checksums;

private ContainerReplica(ContainerReplicaBuilder b) {
this.containerID = Objects.requireNonNull(b.containerID, "containerID == null");
Expand All @@ -57,7 +57,7 @@ private ContainerReplica(ContainerReplicaBuilder b) {
this.replicaIndex = b.replicaIndex;
this.isEmpty = b.isEmpty;
this.sequenceId = b.sequenceId;
this.dataChecksum = b.dataChecksum;
this.checksums = Objects.requireNonNull(b.checksums, "checksums == null");
}

public ContainerID getContainerID() {
Expand Down Expand Up @@ -122,8 +122,12 @@ public boolean isEmpty() {
return isEmpty;
}

public ContainerChecksums getChecksums() {
return checksums;
}

public long getDataChecksum() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return dataChecksum;
return checksums.getDataChecksum();
}

@Override
Expand Down Expand Up @@ -180,7 +184,8 @@ public ContainerReplicaBuilder toBuilder() {
.setOriginNodeId(originDatanodeId)
.setReplicaIndex(replicaIndex)
.setSequenceId(sequenceId)
.setEmpty(isEmpty);
.setEmpty(isEmpty)
.setChecksums(checksums);
}

@Override
Expand All @@ -194,7 +199,7 @@ public String toString() {
+ ", keyCount=" + keyCount
+ ", bytesUsed=" + bytesUsed
+ ", " + (isEmpty ? "empty" : "non-empty")
+ ", dataChecksum=" + dataChecksum
+ ", checksums=" + checksums
+ '}';
}

Expand All @@ -212,7 +217,7 @@ public static class ContainerReplicaBuilder {
private long keyCount;
private int replicaIndex;
private boolean isEmpty;
private long dataChecksum;
private ContainerChecksums checksums;

/**
* Set Container Id.
Expand Down Expand Up @@ -287,8 +292,8 @@ public ContainerReplicaBuilder setEmpty(boolean empty) {
return this;
}

public ContainerReplicaBuilder setDataChecksum(long dataChecksum) {
this.dataChecksum = dataChecksum;
public ContainerReplicaBuilder setChecksums(ContainerChecksums checksums) {
this.checksums = checksums;
return this;
}

Expand All @@ -298,6 +303,9 @@ public ContainerReplicaBuilder setDataChecksum(long dataChecksum) {
* @return ContainerReplicaBuilder
*/
public ContainerReplica build() {
if (this.checksums == null) {
this.checksums = ContainerChecksums.unknown();
}
return new ContainerReplica(this);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.concurrent.TimeUnit;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State;
import org.apache.hadoop.hdds.scm.container.ContainerChecksums;
import org.apache.hadoop.ozone.container.common.interfaces.Container;
import org.apache.hadoop.ozone.container.keyvalue.TestContainerCorruptions;
import org.apache.hadoop.ozone.container.ozoneimpl.BackgroundContainerDataScanner;
Expand Down Expand Up @@ -82,8 +83,8 @@ void testCorruptionDetected(TestContainerCorruptions corruption)
assertNotEquals(0, container.getContainerData().getDataChecksum());

waitForScmToSeeReplicaState(containerID, CLOSED);
long initialReportedDataChecksum = getContainerReplica(containerID).getDataChecksum();
assertNotEquals(0, initialReportedDataChecksum);
ContainerChecksums initialReportedChecksum = getContainerReplica(containerID).getChecksums();
assertNotEquals(ContainerChecksums.unknown(), initialReportedChecksum);
corruption.applyTo(container);

resumeScanner();
Expand All @@ -95,16 +96,16 @@ void testCorruptionDetected(TestContainerCorruptions corruption)

// Wait for SCM to get a report of the unhealthy replica with a different checksum than before.
waitForScmToSeeReplicaState(containerID, UNHEALTHY);
long newReportedDataChecksum = getContainerReplica(containerID).getDataChecksum();
ContainerChecksums newReportedChecksum = getContainerReplica(containerID).getChecksums();
if (corruption == TestContainerCorruptions.MISSING_METADATA_DIR ||
corruption == TestContainerCorruptions.MISSING_CONTAINER_DIR) {
// In these cases, the new tree will not be able to be written since it exists in the metadata directory.
// When the tree write fails, the in-memory checksum should remain at its original value.
assertEquals(checksumToString(initialReportedDataChecksum), checksumToString(newReportedDataChecksum));
assertEquals(initialReportedChecksum, newReportedChecksum);
} else {
assertNotEquals(checksumToString(initialReportedDataChecksum), checksumToString(newReportedDataChecksum));
assertNotEquals(initialReportedChecksum, newReportedChecksum);
// Test that the scanner wrote updated checksum info to the disk.
assertReplicaChecksumMatches(container, newReportedDataChecksum);
assertReplicaChecksumMatches(container, newReportedChecksum);
assertFalse(container.getContainerData().needsDataChecksum());
}

Expand All @@ -118,10 +119,11 @@ void testCorruptionDetected(TestContainerCorruptions corruption)
}
}

private void assertReplicaChecksumMatches(Container<?> container, long expectedChecksum) throws Exception {
private void assertReplicaChecksumMatches(
Container<?> container, ContainerChecksums expectedChecksum) throws Exception {
assertTrue(containerChecksumFileExists(container.getContainerData().getContainerID()));
long dataChecksumFromFile = readChecksumFile(container.getContainerData())
.getContainerMerkleTree().getDataChecksum();
assertEquals(checksumToString(expectedChecksum), checksumToString(dataChecksumFromFile));
assertEquals(checksumToString(expectedChecksum.getDataChecksum()), checksumToString(dataChecksumFromFile));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ public boolean isEmpty() {
return numKeys == 0;
}

public boolean isDataChecksumMismatched() {
public boolean areChecksumsMismatched() {
return !replicas.isEmpty() && replicas.stream()
.map(ContainerReplica::getDataChecksum)
.map(ContainerReplica::getChecksums)
.distinct()
.count() != 1;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ private void processContainer(ContainerInfo container, long currentTime,
containerReplicas, placementPolicy,
reconContainerMetadataManager, conf);

if ((h.isHealthilyReplicated() && !h.isDataChecksumMismatched()) || h.isDeleted()) {
if ((h.isHealthilyReplicated() && !h.areChecksumsMismatched()) || h.isDeleted()) {
return;
}
// For containers deleted in SCM, we sync the container state here.
Expand Down Expand Up @@ -563,7 +563,7 @@ public static List<UnhealthyContainers> generateUnhealthyRecords(
Map<UnHealthyContainerStates, Map<String, Long>>
unhealthyContainerStateStatsMap) {
List<UnhealthyContainers> records = new ArrayList<>();
if ((container.isHealthilyReplicated() && !container.isDataChecksumMismatched()) || container.isDeleted()) {
if ((container.isHealthilyReplicated() && !container.areChecksumsMismatched()) || container.isDeleted()) {
return records;
}

Expand Down Expand Up @@ -610,7 +610,7 @@ public static List<UnhealthyContainers> generateUnhealthyRecords(
populateContainerStats(container, UnHealthyContainerStates.OVER_REPLICATED, unhealthyContainerStateStatsMap);
}

if (container.isDataChecksumMismatched()
if (container.areChecksumsMismatched()
&& !recordForStateExists.contains(
UnHealthyContainerStates.REPLICA_MISMATCH.toString())) {
records.add(recordForState(
Expand Down Expand Up @@ -686,7 +686,7 @@ private static boolean keepMisReplicatedRecord(

private static boolean keepReplicaMismatchRecord(
ContainerHealthStatus container, UnhealthyContainersRecord rec) {
if (container.isDataChecksumMismatched()) {
if (container.areChecksumsMismatched()) {
updateExpectedReplicaCount(rec, container.getReplicationFactor());
updateActualReplicaCount(rec, container.getReplicaCount());
updateReplicaDelta(rec, container.replicaDelta());
Expand Down
Loading