-
Notifications
You must be signed in to change notification settings - Fork 621
HDDS-13305. Create wrapper object for container checksums #8789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
8f59ac0
3323de2
902997d
1299510
f0e922f
0253d2d
ed449d4
c9425f4
242f2e4
cffc5c2
620efb0
42d61cf
5fd256e
f4ef042
df89c7f
d5cf9d9
bb8bb9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| /** | ||
| * Wrapper for container checksums (data, metadata, etc.). | ||
| * Provides equality, hash, and hex string rendering. | ||
| */ | ||
| public class ContainerChecksums { | ||
|
echonesis marked this conversation as resolved.
Outdated
|
||
| private final long dataChecksum; | ||
| private final Long metadataChecksum; // nullable for future use | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the recently merged #8565 for how we are planning to handle unset checksums. We can add this handling into the |
||
|
|
||
| public ContainerChecksums(long dataChecksum) { | ||
| this(dataChecksum, null); | ||
| } | ||
|
|
||
| public ContainerChecksums(long dataChecksum, Long metadataChecksum) { | ||
| this.dataChecksum = dataChecksum; | ||
| this.metadataChecksum = metadataChecksum; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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); |
||
|
|
||
| public long getDataChecksum() { | ||
| return dataChecksum; | ||
| } | ||
|
|
||
| public Long getMetadataChecksum() { | ||
| return metadataChecksum; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (!(obj instanceof ContainerChecksums)) { | ||
| return false; | ||
| } | ||
| ContainerChecksums that = (ContainerChecksums) obj; | ||
| return dataChecksum == that.dataChecksum && | ||
| Objects.equals(metadataChecksum, that.metadataChecksum); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(dataChecksum, metadataChecksum); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append("data=").append(Long.toHexString(dataChecksum)); | ||
| if (metadataChecksum != null) { | ||
| sb.append(", metadata=").append(Long.toHexString(metadataChecksum)); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I will set the default metadataChecksum value to be |
||
| return sb.toString(); | ||
| } | ||
| } | ||
| 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.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class ContainerChecksumsTest { | ||
|
echonesis marked this conversation as resolved.
Outdated
|
||
| @Test | ||
| void testEqualsAndHashCode() { | ||
| ContainerChecksums c1 = new ContainerChecksums(123L); | ||
| ContainerChecksums c2 = new ContainerChecksums(123L); | ||
| ContainerChecksums c3 = new ContainerChecksums(456L); | ||
| ContainerChecksums c4 = new ContainerChecksums(123L, 789L); | ||
| ContainerChecksums c5 = new ContainerChecksums(123L, 789L); | ||
| ContainerChecksums c6 = new ContainerChecksums(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 = new ContainerChecksums(0x1234ABCDL); | ||
| assertTrue(c1.toString().contains("data=1234abcd")); | ||
| assertFalse(c1.toString().contains("metadata=")); | ||
|
echonesis marked this conversation as resolved.
Outdated
|
||
|
|
||
| ContainerChecksums c2 = new ContainerChecksums(0x1234ABCDL, 0xDEADBEEFL); | ||
| assertTrue(c2.toString().contains("data=1234abcd")); | ||
| assertTrue(c2.toString().contains("metadata=deadbeef")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1205,7 +1205,7 @@ public void testWithNoContainerDataChecksum() throws Exception { | |
|
|
||
| // 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could reduce change by keeping the |
||
| assertTrue(contOneDataChecksumsEmpty, "Replicas of container one should not yet have any data checksums."); | ||
|
|
||
| // Send a report to SCM from one datanode that still does not have a data checksum. | ||
|
|
@@ -1222,7 +1222,7 @@ public void testWithNoContainerDataChecksum() throws Exception { | |
| // Regardless of which datanode sent the report, none of them have checksums, so all replica's data checksums | ||
| // should remain empty. | ||
| boolean containerDataChecksumEmpty = containerManager.getContainerReplicas(contID).stream() | ||
| .allMatch(r -> r.getDataChecksum() == 0); | ||
| .allMatch(r -> r.getChecksums().getDataChecksum() == 0); | ||
| assertTrue(containerDataChecksumEmpty, "Replicas of the container should not have any data checksums."); | ||
| } | ||
|
|
||
|
|
@@ -1254,7 +1254,7 @@ public void testWithContainerDataChecksum() throws Exception { | |
|
|
||
| // All replicas should start with an empty data checksum in SCM. | ||
| boolean dataChecksumsEmpty = containerManager.getContainerReplicas(contID).stream() | ||
| .allMatch(r -> r.getDataChecksum() == 0); | ||
| .allMatch(r -> r.getChecksums().getDataChecksum() == 0); | ||
| assertTrue(dataChecksumsEmpty, "Replicas of container one should not yet have any data checksums."); | ||
|
|
||
| // For each datanode, send a container report with a mismatched checksum. | ||
|
|
@@ -1278,7 +1278,7 @@ public void testWithContainerDataChecksum() throws Exception { | |
| int numReplicasChecked = 0; | ||
| for (ContainerReplica replica: containerManager.getContainerReplicas(contID)) { | ||
| long expectedChecksum = createUniqueDataChecksumForReplica(contID, replica.getDatanodeDetails().getUuidString()); | ||
| assertEquals(expectedChecksum, replica.getDataChecksum()); | ||
| assertEquals(expectedChecksum, replica.getChecksums().getDataChecksum()); | ||
| numReplicasChecked++; | ||
| } | ||
| assertEquals(numNodes, numReplicasChecked); | ||
|
|
@@ -1304,7 +1304,7 @@ public void testWithContainerDataChecksum() throws Exception { | |
| numReplicasChecked = 0; | ||
| for (ContainerReplica replica: containerManager.getContainerReplicas(contID)) { | ||
| long expectedChecksum = createMatchingDataChecksumForReplica(contID); | ||
| assertEquals(expectedChecksum, replica.getDataChecksum()); | ||
| assertEquals(expectedChecksum, replica.getChecksums().getDataChecksum()); | ||
| numReplicasChecked++; | ||
| } | ||
| assertEquals(numNodes, numReplicasChecked); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,7 +85,7 @@ void testCorruptionDetected(TestContainerCorruptions corruption) | |
| assertTrue(containerChecksumFileExists(containerID)); | ||
|
|
||
| waitForScmToSeeReplicaState(containerID, CLOSED); | ||
| long initialReportedDataChecksum = getContainerReplica(containerID).getDataChecksum(); | ||
| long initialReportedDataChecksum = getContainerReplica(containerID).getChecksums().getDataChecksum(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this use case we only need to use the There is a new
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. |
||
|
|
||
| corruption.applyTo(container); | ||
|
|
||
|
|
@@ -98,7 +98,7 @@ 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(); | ||
| long newReportedDataChecksum = getContainerReplica(containerID).getChecksums().getDataChecksum(); | ||
| 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this class on the datanode too, so it needs to be in a more general package.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
I will move it into the path under
hadoop-hdds/common.