[
https://issues.apache.org/jira/browse/HDDS-17445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17445:
------------------------------
Affects Version/s: 2.3.0
> Short-circuit GetBlock parks the opened block file in a map shared by all
> requests instead of handing it to the request's own context
> -------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17445
> URL: https://issues.apache.org/jira/browse/HDDS-17445
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Priority: Major
> Attachments: MC-3-hand-short-circuit-block-file-to-own-request.patch
>
>
> h3. Mechanism
> For a GetBlock with {{requestShortCircuitAccess}} set,
> {{KeyValueHandler.handleGetBlock}} opens the block file
> ({{ChunkManager.getShortCircuitFd}}), parks the {{RandomAccessFile}} in
> {{blockFileMap}} under {{BlockUtils.getBlockMapKey}} ({{clientId}},
> {{callId}}, local ID, container ID) and sets {{shortCircuitAccessGranted}} in
> the response. The handler does this for any GetBlock that carries the flag,
> because it does not know which path dispatched the request, and the map is
> shared by all requests. Only the domain socket {{Receiver}} takes the file
> out again, through {{Handler.getBlockFile}} after dispatch, and closes it
> after sending the descriptor (HDDS-11627, HDDS-12883).
> So the hand off between the handler and {{Receiver}} depends on a lookup by
> request fields rather than on the request itself:
> * A GetBlock with the flag dispatched by another path
> ({{GrpcXceiverService}}, {{ContainerStateMachine.query}}) parks a file that
> nothing takes out, and its response reports
> {{shortCircuitAccessGranted=true}} although that path does not pass a
> descriptor.
> * Requests with the same key use the same entry: a later put replaces an
> earlier file without closing it, and the {{Receiver}} task that looks second
> finds no entry.
> A file that is parked and never taken out stays open while the map holds it,
> and a file replaced by a later put is closed only when it is collected.
> h3. Reproduction
> No standalone reproduction is attached. The regression test in the patch
> covers both cases at the handler level.
> h3. Patch
> [^MC-3-hand-short-circuit-block-file-to-own-request.patch], against
> 69d77beff60b9106e2517c36a7a1b2cc726af568. It also applies to master at
> 1acf315d546.
> The opened file is handed to the request's own context instead of a shared
> map, and the flag is ignored on every other path. {{Receiver}} dispatches
> each request with its own {{DispatcherContext}} of the new op
> {{HANDLE_SHORT_CIRCUIT_READ}}; {{handleGetBlock}} opens the file only when it
> gets such a context, stores it there, and {{Receiver}} takes it from that
> context. A GetBlock with the flag dispatched without that context gets the
> block data and {{shortCircuitAccessGranted=false}}, which the read path
> already treats as "read normally". {{blockFileMap}},
> {{KeyValueHandler.getBlockFile}} and the abstract {{Handler.getBlockFile}}
> are removed. The block token is still verified for the new op
> ({{Op.validateToken}} defaults to true), and there is no wire or protocol
> change.
> Not changed: a successful GetBlock without the flag on the domain socket
> still has no file to send; that is the separate issue HDDS-17443. This patch
> and the patch of HDDS-17443 change adjacent lines in
> {{Receiver.ProcessRequestTask.run}}, so whichever is applied second needs a
> trivial rebase.
> Covered by
> {{TestKeyValueHandler.testShortCircuitGetBlockHandsFileToOwnContext}}: a
> GetBlock with the flag dispatched without the domain socket context gets
> {{shortCircuitAccessGranted=false}} and counts no local bytes, a domain
> socket context without the flag gets no file, and two dispatches of the same
> request with their own contexts each get their own open file. On unmodified
> source the first part fails with "Expecting value to be false but was true";
> the other two parts use the new context and do not compile there.
> {{testGetBlockMetrics}} now takes the file from the context, and
> {{TestHddsDispatcher}} lists the new op among those whose token is verified.
> With the patch {{TestKeyValueHandler}},
> {{TestKeyValueHandlerWithUnhealthyContainer}} and {{TestHddsDispatcher}} pass
> (129 tests), as do {{TestXceiverServerDomainSocket}},
> {{TestXceiverClientManagerSC}}, {{TestLocalChunkInputStream}} and
> {{TestDomainSocketFactory}} (17 tests), and checkstyle on
> {{hadoop-hdds/container-service}} is clean.
> Found by TLA+ model checking and code review of the datanode short-circuit
> read paths under HDDS-15926, on commit
> 69d77beff60b9106e2517c36a7a1b2cc726af568. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-11627 (added GetBlock on the short-circuit channel and
> {{blockFileMap}}), HDDS-12883 (closes the block file after it is sent),
> HDDS-16918 (fallback to gRPC is not logged on the datanode), HDDS-16335
> (concurrent access on the other side of the domain socket). 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]
