smengcl commented on code in PR #10496:
URL: https://github.com/apache/ozone/pull/10496#discussion_r3670086801
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1962,7 +1975,18 @@ private long reconcileChunksPerBlock(KeyValueContainer
container, Pipeline pipel
if (!localOffset2Chunk.isEmpty()) {
List<ContainerProtos.ChunkInfo> allChunks = new
ArrayList<>(localOffset2Chunk.values());
localBlockData.setChunks(allChunks);
- putBlockForClosedContainer(container, localBlockData, maxBcsId,
allChunksSuccessful);
+ // The peer's BCSID attests exactly the chunk list in its committed
BlockData, so that list is the oracle for
+ // adopting it -- not the diff-derived peerChunkList, which omits
chunks the peer's scanner marked unhealthy
+ // (and the in-loop unhealthy skip above does not clear
allChunksSuccessful). Without this check a trailing
+ // unrepairable peer chunk lets the BCSID advance past data we do not
hold: the replica would then admit
+ // reads it cannot serve and look complete to SCM's sequenceId-based
source and delete selection.
+ boolean adoptPeerBcsId = allChunksSuccessful &&
coversPeerBlock(peerBlockData, localOffset2Chunk);
+ if (allChunksSuccessful && !adoptPeerBcsId) {
+ LOG.warn("Repaired all {} diff chunks for block {} in container {}
from peer {}, but the local block does " +
+ "not cover the peer's committed chunk list. BCSID stays at the
local value.",
+ peerChunkList.size(), localID, containerID, peer);
+ }
+ putBlockForClosedContainer(container, localBlockData, maxBcsId,
adoptPeerBcsId);
Review Comment:
`coversPeerBlock` proves completeness only for this block, but
`adoptPeerBcsId` is also passed to
`putBlockForClosedContainer`, which uses it to advance the container-wide
BCSID. Another block from the same peer can
remain incomplete at a lower BCSID while a later fully covered block passes
this gate and advances the container past
that missing data.
It can be reproed by adding this regression to
`TestReconcileChunksPerBlockHoleBcsId`:
```java
@Test
public void completeBlockMustNotAdvanceContainerPastIncompleteEarlierBlock()
throws Exception {
long incompleteBlockId = 1L;
long completeBlockId = 2L;
long incompleteBlockBcsId = PEER_BCSID - 1;
ContainerProtos.ChunkInfo incompleteChunk0 =
chunkProto("incomplete-0", 0, (byte) 'b');
ContainerProtos.ChunkInfo incompleteChunk1 =
chunkProto("incomplete-1", CHUNK_LEN, (byte) 'c');
installMockedPeerStream(peerBlockDataWithChunks(
incompleteBlockId, incompleteBlockBcsId, incompleteChunk0,
incompleteChunk1), 0);
handler.reconcileChunksPerBlock(
container, peerPipeline, dnClient, incompleteBlockId,
Arrays.asList(chunkMerkleTree(0), unhealthyChunkMerkleTree(CHUNK_LEN)),
new ContainerMerkleTreeWriter(), ByteBuffer.allocate(CHUNK_LEN));
BlockData incompleteAfter = handler.getBlockManager().getBlock(
container, new BlockID(CONTAINER_ID, incompleteBlockId));
assertEquals(1, incompleteAfter.getChunks().size());
assertEquals(LOCAL_BCSID,
container.getContainerData().getBlockCommitSequenceId());
ContainerProtos.ChunkInfo completeChunk0 =
chunkProto("complete-0", 0, (byte) 'b');
installMockedPeerStream(peerBlockDataWithChunks(
completeBlockId, PEER_BCSID, completeChunk0), 0);
handler.reconcileChunksPerBlock(
container, peerPipeline, dnClient, completeBlockId,
Collections.singletonList(chunkMerkleTree(0)),
new ContainerMerkleTreeWriter(), ByteBuffer.allocate(CHUNK_LEN));
BlockData completeAfter = handler.getBlockManager().getBlock(
container, new BlockID(CONTAINER_ID, completeBlockId));
assertEquals(PEER_BCSID, completeAfter.getBlockCommitSequenceId());
assertEquals(
LOCAL_BCSID,
container.getContainerData().getBlockCommitSequenceId(),
"container BCSID must not advance past the incomplete block");
}
```
On the current head, the final assertion fails with `expected: <1> but was:
<99>`.
Could we separate per-block BCSID adoption from the container BCSID update?
The block can adopt the peer BCSID once
`coversPeerBlock` passes, but the container BCSID should be deferred until
reconciliation establishes container-level
coverage, such as when the post-repair container checksum matches the peer
snapshot. Please add the two-block regression
above.
--
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]
