kerneltime commented on code in PR #10496:
URL: https://github.com/apache/ozone/pull/10496#discussion_r3668842256
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1895,6 +1902,11 @@ private long reconcileChunksPerBlock(KeyValueContainer
container, Pipeline pipel
for (ContainerProtos.ChunkMerkleTree chunkMerkleTree : peerChunkList) {
long chunkOffset = chunkMerkleTree.getOffset();
if (!previousChunkPresent(blockID, chunkOffset, localOffset2Chunk)) {
+ // A hole remains: the chunk preceding this offset is missing
locally, so the block stays
+ // incomplete. Treat this like the per-chunk failure path below so
the commit does not
+ // overwrite the block/container BCSID with the peer's value while
data past the hole is
+ // absent. Advancing the BCSID here would falsely advertise
committed data we do not hold.
+ allChunksSuccessful = false;
Review Comment:
Thanks for the careful analysis -- you're right, and I verified every link
in the chain. Fixed in ac5ce463d0.
You identified the filtered-trailing-chunk lane; while confirming it I found
a second lane with the same escape: the missing-block path passes the peer's
full chunk list unfiltered (`addMissingBlock` does not health-check), and the
in-loop unhealthy skip (`continue`) does not fail the repair. An interior skip
is still caught by the next chunk's `previousChunkPresent` check, but a
trailing skip has no successor, so `allChunksSuccessful` stays true. Both lanes
are trailing-only escapes -- adjacency checks are inherently blind at the tail.
Between your two options I went with verifying the resulting block at commit
time, for a concrete reason: propagating completeness through the diff would
not cover the second lane (it never passes through the filter), and the method
already holds the right oracle -- `blockInputStream.getStreamBlockData()`, the
peer's committed BlockData. That chunk list is precisely what the peer's BCSID
attests, and because the getBlock snapshot carries the chunk list and the BCSID
atomically, the same gate also closes the HDDS-12986 interleaving (stale peer
merkle tree vs fresh peer BCSID). Repair behavior is unchanged: recoverable
chunks are still ingested and committed, only the BCSID adoption is gated
(`coversPeerBlock`).
Tests: your repro is now a regression driven through a real
`ContainerChecksumTreeManager.diff()` -- it pins the filter dropping the
trailing chunk, asserts the healthy chunk is still recovered, and asserts both
block and container BCSID stay at the exact local value. A second regression
covers the unfiltered skip lane, and a positive control asserts the BCSID still
advances when the local block fully covers the peer's chunk list. Both
regressions fail with `expected: <1> but was: <99>` without the gate.
Also filed HDDS-16011 to formalize the BCSID semantics during reconciliation
(design doc, plus the tree-equality convergence heuristic deferred from the
#7474 review) and HDDS-16012 for an SCM-side invariant check (equal BCSID with
mismatched data checksums) that would surface this bug class in live clusters.
--
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]
