Siyao Meng created HDDS-17445:
---------------------------------
Summary: 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
Reporter: Siyao Meng
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]
