kerneltime commented on code in PR #10496:
URL: https://github.com/apache/ozone/pull/10496#discussion_r3717792819
##########
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:
Confirmed with your repro exactly as written (red with `expected: <1> but
was: <99>`), and fixed in fcd21185c4 by the separation you suggested: per-block
repair never touches the container BCSID; `reconcileContainerInternal`
accumulates coverage across the round and advances the container in one
round-end step only when no repaired block stayed partial. A clean round
converges exactly as before, so recovery availability is unchanged.
Your finding prompted a sweep for remaining lanes of this class, which found
one more (df197124fc): a block whose *only* differences are peer-unhealthy
chunks vanishes from the diff entirely -- `reportChunkIfHealthy` drops its
entries, so it can never dirty the round through a partial repair. That is
literally round two of your own scenario, after round one has correctly
recovered everything recoverable. `ContainerDiffReport` now counts filter drops
and the round-end advancement also requires zero drops. The one remaining lane
-- data absent from both replicas' trees -- is invisible to any diff by
construction; advancing there inherits the peer's own container claim at the
same trust level as wholesale replication, and is documented as the accepted
boundary in the helper javadoc.
On the tree-match variant you floated: it needs the peer's *container*
BCSID, which `getContainerChecksumInfo` deliberately does not carry (the #7474
review deferred exactly that -- it would take a readContainer call or a proto
change). Round-coverage needs no new RPC and is strictly conservative. The
fuller tree-equality adoption stays scoped in HDDS-16011.
Tests: your two-block regression essentially verbatim plus a round-end
suppression assertion, the filtered-only-block regression driving the real
`diff()`, and the positive control extended across the round-end seam. All fail
without their respective guards; full run 181/181 across the reconcile, diff,
checksum-manager, and mock-datanode suites.
--
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]
