[
https://issues.apache.org/jira/browse/HDDS-17676?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17676:
------------------------------
Attachment: CR-8-finalize-block-keeps-stored-bcsid.patch
> FinalizeBlock lowers the stored block bcsId to the request's when the block
> has no last_chunk_info row
> ------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17676
> URL: https://issues.apache.org/jira/browse/HDDS-17676
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Priority: Minor
> Attachments: CR-8-finalize-block-keeps-stored-bcsid.patch,
> TestBugCR8FinalizeBlockBcsId.java
>
>
> h3. Mechanism
> With the incremental chunk list, {{BlockManagerImpl.finalizeBlock}} merges
> the last chunk into the block through {{mergeLastChunkForBlockFinalization}}.
> It passes {{new BlockData(blockId)}}, built from the request, to
> {{putBlockByID}} with end of block set, which ends in
> {{DatanodeStoreWithIncrementalChunkList.moveLastChunkToBlockData}}:
> * If a {{last_chunk_info}} row exists, {{reconcilePartialChunks}} sets the
> bcsId from the row, which is correct.
> * If there is no row, case 2.2 copies the bcsId of the request onto the
> stored {{block_data}} row.
> There is no row after a PutBlock whose last chunk is full. Lease recovery
> sends FinalizeBlock with the {{BlockID}} that OM holds, which can carry an
> older bcsId than the last PutBlock (the
> {{KeyValueHandler.handleFinalizeBlock}} check only rejects a higher one).
> FinalizeBlock goes through Ratis, so the stored block bcsId goes down on
> every replica. The container BCSID is not changed.
> Introduced on master by HDDS-16730, which made the merge persistent. Before
> it, {{finalizeBlock}} committed its batch before the merge, so no release is
> affected.
> h3. Trigger
> # HBase enhancements, hsync and {{ozone.client.incremental.chunk.list}} are
> enabled (all default to false).
> # A writer hsyncs, then writes until the current chunk is full and flushes,
> so the last PutBlock leaves no {{last_chunk_info}} row.
> # The writer stops and {{recoverLease}} finalizes the block with the bcsId
> from OM.
> h3. Impact
> No observable effect found: readers and lease recovery use the bcsId that OM
> holds, which after the recovery equals the lowered stored value, and
> reconciliation takes the larger bcsId of two replicas. A {{BCSID_MISMATCH}}
> would need a GetBlock with the pre finalize bcsId, which no client path sends
> today. This is filed as a follow up to HDDS-16730, which is not released yet:
> fixing it before that change ships means no release ever saves a lowered
> block bcsId on disk.
> h3. Reproduction
> Fails on the unfixed code, deterministic, unmodified source.
> [^TestBugCR8FinalizeBlockBcsId.java] ({{hadoop-ozone/integration-test}})
> starts 3 datanodes, writes through ofs with the incremental chunk list,
> hsyncs at one chunk + 1024 bytes, fills the second chunk and flushes,
> recovers the lease, reads the file back and then expects each replica's
> stored block bcsId to be unchanged by the recovery. It fails on all 3
> replicas: the stored bcsId goes from 8 to 0, the value OM held for the block.
> The read back before that assertion passes. With the patch it passes, and
> {{TestLeaseRecovery}} passes as well.
> h3. Patch
> [^CR-8-finalize-block-keeps-stored-bcsid.patch], against
> ec5ed5f7ea0e4b406aee53aae4906616819cba2b. It also applies to master at
> 7294fc2da18.
> {{mergeLastChunkForBlockFinalization}} already reads the stored block; the
> merge input now takes that block's bcsId instead of the request's. When a
> {{last_chunk_info}} row exists the row's bcsId still wins, so only the case
> without a row changes. {{moveLastChunkToBlockData}} is left as is because it
> also serves every incremental PutBlock.
> Covered by {{testFinalizeBlockKeepsBcsIdWhenLastChunkIsFull}} in the existing
> {{TestBlockManagerImpl}}, next to {{testFinalizeBlockMergesLastChunk}} which
> covers the case with a row. Without the change it fails with {{expected: <2>
> but was: <0>}}. With it {{TestBlockManagerImpl}}, {{TestKeyValueHandler}},
> {{TestKeyValueContainerCheck}} and
> {{TestContainerReconciliationWithMockDatanodes}} (196 tests) pass.
> Found by code review of the datanode container state machine apply and block
> commit paths, as part of the TLA+ verification effort under HDDS-15926, on
> commit ec5ed5f7ea0e4b406aee53aae4906616819cba2b. Checked against HDDS issues
> and apache/ozone pull requests for duplicates before filing. The attached
> patch is a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
