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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
