You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "echonesis (via GitHub)" <gi...@apache.org> on 2025/12/15 12:40:58 UTC
[PR] HDDS-13305. Create wrapper object for container checksums [ozone]
echonesis opened a new pull request, #8789:
URL: https://github.com/apache/ozone/pull/8789
## 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
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2227916164
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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;
Review Comment:
Got it.
I will move it into the path under `hadoop-hdds/common`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3106003498
> 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2224401944
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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 {
Review Comment:
Got it.
Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2630405870
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java:
##########
@@ -84,23 +85,29 @@ public void setState(String state) {
}
public long getDataChecksum() {
- return dataChecksum;
+ return getChecksums().getDataChecksum();
}
- public void setDataChecksum(long dataChecksum) {
- this.dataChecksum = dataChecksum;
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
+ public void setChecksums(ContainerChecksums checksums) {
+ this.checksums = checksums != null ? checksums : ContainerChecksums.unknown();
}
public static ContainerReplicaHistory fromProto(
ContainerReplicaHistoryProto proto) {
return new ContainerReplicaHistory(UUID.fromString(proto.getUuid()),
proto.getFirstSeenTime(), proto.getLastSeenTime(), proto.getBcsId(),
- proto.getState(), proto.getDataChecksum());
+ proto.getState(), ContainerChecksums.of(proto.getDataChecksum(), 0L));
Review Comment:
Thanks @adoroszlai for pointing this out and pushing the commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2627742954
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
+
+ // 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()) {
+ 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()) {
+ 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);
Review Comment:
I agree that we should consider this unequality.
In https://github.com/apache/ozone/pull/8565, it seems the `UNSET` placeholder could help us separate the 2 states.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #8789:
URL: https://github.com/apache/ozone/pull/8789
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2226847545
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java:
##########
@@ -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();
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2619280549
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
+
+ // 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()) {
+ 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()) {
Review Comment:
nit: this should mention `UNSET_METADATA_CHECKSUM`, also please move comment to `UNSET_METADATA_CHECKSUM` definition
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
+
+ // 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()) {
+ 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()) {
+ 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);
Review Comment:
`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.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
Review Comment:
nit: use `UNSET_DATA_CHECKSUM` and `UNSET_METADATA_CHECKSUM`
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java:
##########
@@ -84,23 +85,29 @@ public void setState(String state) {
}
public long getDataChecksum() {
- return dataChecksum;
+ return getChecksums().getDataChecksum();
}
- public void setDataChecksum(long dataChecksum) {
- this.dataChecksum = dataChecksum;
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
+ public void setChecksums(ContainerChecksums checksums) {
+ this.checksums = checksums;
}
public static ContainerReplicaHistory fromProto(
ContainerReplicaHistoryProto proto) {
return new ContainerReplicaHistory(UUID.fromString(proto.getUuid()),
proto.getFirstSeenTime(), proto.getLastSeenTime(), proto.getBcsId(),
- proto.getState(), proto.getDataChecksum());
+ proto.getState(), ContainerChecksums.of(proto.getDataChecksum(), 0L));
}
public ContainerReplicaHistoryProto toProto() {
return ContainerReplicaHistoryProto.newBuilder().setUuid(uuid.toString())
.setFirstSeenTime(firstSeenTime).setLastSeenTime(lastSeenTime)
- .setBcsId(bcsId).setState(state).setDataChecksum(dataChecksum).build();
+ .setBcsId(bcsId).setState(state)
+ .setDataChecksum(checksums != null ? checksums.getDataChecksum() : 0L)
Review Comment:
```suggestion
.setDataChecksum(checksums.getDataChecksum())
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
+
+ // 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()) {
Review Comment:
nit: move comment to `UNSET_DATA_CHECKSUM` definition
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java:
##########
@@ -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))
Review Comment:
Please replace all calls like this with `ContainerChecksums.dataOnly(...)` as [suggested earlier](https://github.com/apache/ozone/pull/8789#discussion_r2221522284).
Rationale:
- Avoid duplication of 0L as unknown value
- Avoid mismatch between internal-only constant with value -1L
- Easier to understand
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java:
##########
@@ -84,23 +85,29 @@ public void setState(String state) {
}
public long getDataChecksum() {
- return dataChecksum;
+ return getChecksums().getDataChecksum();
}
- public void setDataChecksum(long dataChecksum) {
- this.dataChecksum = dataChecksum;
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
+ public void setChecksums(ContainerChecksums checksums) {
+ this.checksums = checksums;
Review Comment:
```suggestion
this.checksums = checksums != null ? checksums : ContainerChecksums.unknown();
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java:
##########
@@ -39,16 +40,16 @@ public class ContainerReplicaHistory {
private long bcsId;
private String state;
- private long dataChecksum;
+ private ContainerChecksums checksums;
public ContainerReplicaHistory(UUID id, Long firstSeenTime,
- Long lastSeenTime, long bcsId, String state, long dataChecksum) {
+ Long lastSeenTime, long bcsId, String state, ContainerChecksums checksums) {
this.uuid = id;
this.firstSeenTime = firstSeenTime;
this.lastSeenTime = lastSeenTime;
this.bcsId = bcsId;
this.state = state;
- this.dataChecksum = dataChecksum;
+ this.checksums = checksums;
Review Comment:
```suggestion
setChecksums(checksums);
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2626441642
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java:
##########
@@ -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))
Review Comment:
Thanks @adoroszlai
According to the [review](https://github.com/apache/ozone/pull/8789#discussion_r2226709231) 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2627346030
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java:
##########
@@ -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))
Review Comment:
Thanks for pointing it out!
I will modify it in the next commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3514418467
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2221499921
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/ContainerChecksumsTest.java:
##########
@@ -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 {
Review Comment:
This is not run due to naming convention, should be `TestContainerChecksums`.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java:
##########
@@ -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);
Review Comment:
We could reduce change by keeping the `getDataChecksum()` method in `ContainerReplica` and delegate to `checksums`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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 {
Review Comment:
Could be `final`, and please annotate with `@Immutable`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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 {
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
+
+ public ContainerChecksums(long dataChecksum) {
+ this(dataChecksum, null);
+ }
+
+ public ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+ this.dataChecksum = dataChecksum;
+ this.metadataChecksum = metadataChecksum;
+ }
Review Comment:
We could add factory methods (and make constructor private) to reduce accidental mismatch of intended and actual source (data or metadata).
Something like:
```java
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);
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/ContainerChecksumsTest.java:
##########
@@ -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 {
+ @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="));
Review Comment:
nit: for better failure message prefer `assertThat(...).contains(...)` and `.doesNotContain(...)` (can be called in chain, too).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3105881413
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`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3660933730
Thanks @adoroszlai for the review.
I will update them in the next commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2227916164
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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;
Review Comment:
Got it.
I will move it under `hadoop-hdds/common` path.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2627742954
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -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);
+
+ // 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()) {
+ 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()) {
+ 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);
Review Comment:
I agree that we should consider this unequality.
In https://github.com/apache/ozone/pull/8565, it seems the `UNSET` placeholder could help us separate the 2 states:
- UNSET state
- Calculated state
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3573219252
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2226709231
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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(0, null);
+
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
+
+ private ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+ this.dataChecksum = dataChecksum;
+ this.metadataChecksum = metadataChecksum;
+ }
+
+ public static ContainerChecksums unknown() {
+ return UNKNOWN;
+ }
+
+ public static ContainerChecksums dataOnly(long dataChecksum) {
+ return new ContainerChecksums(dataChecksum, null);
+ }
+
+ public static ContainerChecksums metadataOnly(long metadataChecksum) {
Review Comment:
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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplica.java:
##########
@@ -122,8 +122,12 @@ public boolean isEmpty() {
return isEmpty;
}
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
public long getDataChecksum() {
Review Comment:
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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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(0, null);
+
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
Review Comment:
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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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(0, null);
+
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
+
+ private ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+ this.dataChecksum = dataChecksum;
+ this.metadataChecksum = metadataChecksum;
+ }
+
+ public static ContainerChecksums unknown() {
+ return UNKNOWN;
+ }
+
+ public static ContainerChecksums dataOnly(long dataChecksum) {
+ return new ContainerChecksums(dataChecksum, null);
+ }
+
+ public static ContainerChecksums metadataOnly(long metadataChecksum) {
+ return new ContainerChecksums(0, metadataChecksum);
+ }
+
+ public static ContainerChecksums of(long dataChecksum, long metadataChecksum) {
+ return new ContainerChecksums(dataChecksum, 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));
+ }
Review Comment:
`0` should be the displayed value for any checksum that is not set.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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;
Review Comment:
We need this class on the datanode too, so it needs to be in a more general package.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -162,7 +163,8 @@ public boolean isEmpty() {
public boolean isDataChecksumMismatched() {
return !replicas.isEmpty() && replicas.stream()
- .map(ContainerReplica::getDataChecksum)
+ .map(ContainerReplica::getChecksums)
+ .map(ContainerChecksums::getDataChecksum)
.distinct()
.count() != 1;
Review Comment:
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.
```suggestion
public boolean areChecksumsMismatched() {
return !replicas.isEmpty() && replicas.stream()
.map(ContainerReplica::getChecksums)
.distinct()
.count() != 1;
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2227266550
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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(0, null);
+
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
+
+ private ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+ this.dataChecksum = dataChecksum;
+ this.metadataChecksum = metadataChecksum;
+ }
+
+ public static ContainerChecksums unknown() {
+ return UNKNOWN;
+ }
+
+ public static ContainerChecksums dataOnly(long dataChecksum) {
+ return new ContainerChecksums(dataChecksum, null);
+ }
+
+ public static ContainerChecksums metadataOnly(long metadataChecksum) {
+ return new ContainerChecksums(0, metadataChecksum);
+ }
+
+ public static ContainerChecksums of(long dataChecksum, long metadataChecksum) {
+ return new ContainerChecksums(dataChecksum, 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));
+ }
Review Comment:
Got it. I will set the default metadataChecksum value to be `0`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2224240364
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/ContainerChecksumsTest.java:
##########
@@ -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 {
Review Comment:
Thanks @adoroszlai for pointing this out.
I will update it ASAP.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3674697840
Thanks @echonesis for the patch, @errose28 for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2224264329
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/ContainerChecksumsTest.java:
##########
@@ -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 {
+ @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="));
Review Comment:
Thanks!
Updated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2227244988
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerChecksums.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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(0, null);
+
+ private final long dataChecksum;
+ private final Long metadataChecksum; // nullable for future use
+
+ private ContainerChecksums(long dataChecksum, Long metadataChecksum) {
+ this.dataChecksum = dataChecksum;
+ this.metadataChecksum = metadataChecksum;
+ }
+
+ public static ContainerChecksums unknown() {
+ return UNKNOWN;
+ }
+
+ public static ContainerChecksums dataOnly(long dataChecksum) {
+ return new ContainerChecksums(dataChecksum, null);
+ }
+
+ public static ContainerChecksums metadataOnly(long metadataChecksum) {
Review Comment:
Thanks @errose28
I will adjust `ContainerChecksums` constructors to be
1. UNKNOWN (no checksums)
2. 2 checksums included (dataChecksum + metadataChecksum)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2626489752
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java:
##########
@@ -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))
Review Comment:
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()`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2629835060
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ContainerReplicaHistory.java:
##########
@@ -84,23 +85,29 @@ public void setState(String state) {
}
public long getDataChecksum() {
- return dataChecksum;
+ return getChecksums().getDataChecksum();
}
- public void setDataChecksum(long dataChecksum) {
- this.dataChecksum = dataChecksum;
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
+ public void setChecksums(ContainerChecksums checksums) {
+ this.checksums = checksums != null ? checksums : ContainerChecksums.unknown();
}
public static ContainerReplicaHistory fromProto(
ContainerReplicaHistoryProto proto) {
return new ContainerReplicaHistory(UUID.fromString(proto.getUuid()),
proto.getFirstSeenTime(), proto.getLastSeenTime(), proto.getBcsId(),
- proto.getState(), proto.getDataChecksum());
+ proto.getState(), ContainerChecksums.of(proto.getDataChecksum(), 0L));
Review Comment:
```suggestion
proto.getState(), ContainerChecksums.of(proto.getDataChecksum()));
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "echonesis (via GitHub)" <gi...@apache.org>.
echonesis commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2232988781
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java:
##########
@@ -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();
Review Comment:
Got it.
I've updated `assertReplicaChecksumMatches` by replacing `long` with `ContainerChecksums`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #8789:
URL: https://github.com/apache/ozone/pull/8789#discussion_r2227255042
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplica.java:
##########
@@ -122,8 +122,12 @@ public boolean isEmpty() {
return isEmpty;
}
+ public ContainerChecksums getChecksums() {
+ return checksums;
+ }
+
public long getDataChecksum() {
Review Comment:
`long getDataChecksum` was retained on my request to reduce change in existing tests. We don't need to adjust this class for future values.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3137551875
@errose28 please take another look
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8789:
URL: https://github.com/apache/ozone/pull/8789#issuecomment-3669099094
@errose28 please take another look
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13305. Create wrapper object for container checksums [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8789: HDDS-13305. Create wrapper object for container checksums
URL: https://github.com/apache/ozone/pull/8789
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org