Copilot commented on code in PR #10496: URL: https://github.com/apache/ozone/pull/10496#discussion_r3401682017
########## hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestReconcileChunksPerBlockHoleBcsId.java: ########## @@ -0,0 +1,260 @@ +/* + * 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.ozone.container.keyvalue; + +import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS; +import static org.apache.hadoop.hdds.protocol.MockDatanodeDetails.randomDatanodeDetails; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_KEY; +import static org.apache.hadoop.ozone.container.common.ContainerTestUtils.WRITE_STAGE; +import static org.apache.hadoop.ozone.container.common.ContainerTestUtils.createDbInstancesForTestIfNeeded; +import static org.apache.hadoop.ozone.container.common.impl.ContainerImplTestUtils.newContainerSet; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.UUID; +import org.apache.hadoop.hdds.client.BlockID; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import org.apache.hadoop.hdds.scm.storage.BlockInputStream; +import org.apache.hadoop.hdds.scm.storage.ChunkInputStream; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.client.io.BlockInputStreamFactoryImpl; +import org.apache.hadoop.ozone.common.Checksum; +import org.apache.hadoop.ozone.common.ChecksumData; +import org.apache.hadoop.ozone.container.checksum.ContainerMerkleTreeWriter; +import org.apache.hadoop.ozone.container.checksum.DNContainerOperationClient; +import org.apache.hadoop.ozone.container.common.ContainerTestUtils; +import org.apache.hadoop.ozone.container.common.helpers.BlockData; +import org.apache.hadoop.ozone.container.common.helpers.ChunkInfo; +import org.apache.hadoop.ozone.container.common.impl.ContainerSet; +import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet; +import org.apache.hadoop.ozone.container.common.volume.StorageVolume; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Regression test for the BCSID high-water bug on the holed-block reconciliation path + * (KeyValueHandler.reconcileChunksPerBlock). + * + * <p>Scenario reproduced here: + * <ul> + * <li>A closed local replica holds block L with only the offset-0 chunk; its block and container + * blockCommitSequenceId (BCSID) are both 1.</li> + * <li>A peer is ahead at BCSID 99 and advertises a chunk merkle list {CHUNK_LEN, 3*CHUNK_LEN}. + * The chunk at 2*CHUNK_LEN is absent, so 3*CHUNK_LEN sits past a hole. (A peer's scanner + * legitimately omits missing chunks from its merkle tree, so a healthy peer can advertise a + * gapped list.)</li> + * </ul> + * + * <p>Reconciliation ingests the chunk at CHUNK_LEN (its predecessor, offset 0, is present locally), + * then reaches 3*CHUNK_LEN whose predecessor 2*CHUNK_LEN is missing and stops at the hole break. + * The block is therefore incomplete. The method contract states the BCSID is advanced to the peer's + * value only when the entire block is read and written successfully, so on a holed repair the + * BCSID must stay at the local value. + * + * <p>This test mocks only the peer side (the BlockInputStream and its single served chunk) and + * exercises the real reconcileChunksPerBlock against a real closed container. Before the fix the + * BCSID is advanced to 99 and the assertions below fail; after the fix the BCSID stays at 1. + */ +public class TestReconcileChunksPerBlockHoleBcsId { + + @TempDir + private Path tempDir; + + private static final String CLUSTER_ID = UUID.randomUUID().toString(); + private static final long CONTAINER_ID = 100L; + private static final long LOCAL_ID = 0L; + // 2 KiB chunks so the offsets line up with the description: ingested chunk at 2048, + // hole at 4096, skipped chunk past the hole at 6144. + private static final int CHUNK_LEN = 2 * (int) OzoneConsts.KB; + private static final int BYTES_PER_CHECKSUM = 2 * (int) OzoneConsts.KB; + private static final long LOCAL_BCSID = 1L; + private static final long PEER_BCSID = 99L; + + private ContainerSet containerSet; + private KeyValueHandler handler; + private KeyValueContainer container; + private DNContainerOperationClient dnClient; + private Pipeline peerPipeline; + + @BeforeEach + public void setup() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + Path dataVolume = Paths.get(tempDir.toString(), "data"); + Path metadataVolume = Paths.get(tempDir.toString(), "metadata"); + conf.set(HDDS_DATANODE_DIR_KEY, dataVolume.toString()); + conf.set(OZONE_METADATA_DIRS, metadataVolume.toString()); + + containerSet = newContainerSet(); + DatanodeDetails localDn = randomDatanodeDetails(); + MutableVolumeSet volumeSet = new MutableVolumeSet(localDn.getUuidString(), conf, null, + StorageVolume.VolumeType.DATA_VOLUME, null); + createDbInstancesForTestIfNeeded(volumeSet, CLUSTER_ID, CLUSTER_ID, conf); + + handler = ContainerTestUtils.getKeyValueHandler(conf, localDn.getUuidString(), containerSet, volumeSet, + new org.apache.hadoop.ozone.container.checksum.ContainerChecksumTreeManager(conf)); + handler.setClusterID(CLUSTER_ID); + + container = createClosedContainerWithOffsetZeroChunk(); + + dnClient = new DNContainerOperationClient(conf, null, null); + peerPipeline = singleNodePipeline(randomDatanodeDetails()); + } + Review Comment: `DNContainerOperationClient` implements `AutoCloseable` and owns an `XceiverClientManager`. This test creates a new instance in `@BeforeEach` but never closes it, which can leak resources / threads across the test suite. Add an `@AfterEach` teardown to close `dnClient` (and stop the handler to release its managers). -- 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]
