HDDS-15424. Support concurrent positional read - #11102
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in mechanism to make positioned reads (pread) thread-safe in OzoneFS by serializing the seek-read-restore sequence inside OzoneFSInputStream, addressing intermittent checksum failures when multiple threads reuse the same input stream.
Changes:
- Add
ozone.fs.synchronize.positioned.reads.enabledto optionally synchronize positioned reads inOzoneFSInputStream. - Plumb the new flag through the OzoneFS implementations so the created FS input streams honor the configuration.
- Add a unit test that exercises concurrent positioned reads with the flag enabled/disabled.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/RootedOzoneFileSystem.java | Passes the new positioned-read synchronization flag when constructing the FS input stream wrapper. |
| hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/OzoneFileSystem.java | Passes the new positioned-read synchronization flag when constructing the FS input stream wrapper. |
| hadoop-ozone/ozonefs-hadoop3/src/main/java/org/apache/hadoop/fs/ozone/RootedOzoneFileSystem.java | Same flag plumbing for the Hadoop3 variant. |
| hadoop-ozone/ozonefs-hadoop3/src/main/java/org/apache/hadoop/fs/ozone/OzoneFileSystem.java | Same flag plumbing for the Hadoop3 variant. |
| hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSInputStream.java | Adds concurrent positioned-read coverage to validate the opt-in synchronization behavior. |
| hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java | Implements optional serialization of positioned reads via an internal lock. |
| hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/CapableOzoneFSInputStream.java | Extends constructors to propagate the new positioned-read synchronization option. |
| hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java | Reads the new config key and uses it when creating OzoneFSInputStream. |
| hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java | Reads the new config key and uses it when creating OzoneFSInputStream. |
| hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java | Defines the new configuration key and default value. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0b724bf to
ffb81d6
Compare
8293565 to
d45d09d
Compare
|
the om integration test |
| return doPositionedRead(chunkRelativePosition, dst); | ||
| } | ||
| // Local (short-circuit) reads share a FileChannel cursor; serialize them. | ||
| synchronized (this) { |
There was a problem hiding this comment.
Could we synchronize local positioned reads on the shared block FileChannel, or use positional FileChannel reads? Each LocalChunkInputStream locks its own instance, while all chunks in the block receive the same blockFileInputStream, so preads to different chunks can still interleave position(...).read(...) and use the wrong offset.
There was a problem hiding this comment.
yup, you're right about the positional FileChannel, I made the change and please review again.
| int idx = chunkIndexForPosition(pos, offsets); | ||
| ChunkInputStream chunk = streams.get(idx); | ||
| long chunkPos = pos - offsets[idx]; | ||
| int n = chunk.readPositioned(chunkPos, dst); |
There was a problem hiding this comment.
Could we preserve BlockInputStream’s outer retry and refresh handling here? readChunk still tries the DNs in the current pipeline, but this native path cannot refresh an expired block token or fetch an updated pipeline after those attempts fail.
There was a problem hiding this comment.
thanks, this is an important miss.
|
@taklwu thanks for the patch! |
szetszwo
left a comment
There was a problem hiding this comment.
@taklwu , thanks for working on this! BlockInputStream is not designed to support concurrent access. Filed HDDS-16325 to refactor it.
| final long[] offsets = chunkOffsets; | ||
| final BlockData currentBlockData = blockData; | ||
| final long blockLength = length; |
There was a problem hiding this comment.
They need to be synchronized.
There was a problem hiding this comment.
sorry, may ask why it need to be synchronized? I thought chunkOffsets, blockData and length were being initialized once in the initialize() which is a synchronized function already, here the readPositioned implemented as stateless and is trying to getting the the snapshot of this read only information for further operation within readPositioned
maybe I missed something that these three data will be changed over time?
| final int startPosition = dst.position(); | ||
| int preadRetries = 0; | ||
| while (true) { | ||
| final ChunkInputStream chunkStream = createChunkInputStream(chunkInfo); |
There was a problem hiding this comment.
createChunkInputStream uses xceiverClientFactory, xceiverClientGrpc and xceiverClientShortCircuit, which need to be synchronized.
There was a problem hiding this comment.
I rechecked with below, so it seems the synchronized should not be required in the body
gRPC path (blockFileInputStream == null):
- The ephemeral ChunkInputStream gets its own xceiverClient via ChunkInputStream.acquireClient() (synchronized per instance).
- That uses xceiverClientFactory.acquireClientForReadData(), which is thread-safe internally (XceiverClientManager synchronizes on its cache).
- Block-level xceiverClientGrpc / xceiverClientShortCircuit are only used for getBlockData() during init, not for pread data reads.
Short-circuit path:
- LocalChunkInputStream uses positional FileChannel.read(buffer, pos) so concurrent preads on the shared block channel are safe.
- Its acquireClient() is a no-op; it does not use the block’s short-circuit client for reads
but I did checked that the real edge case is pread vs close()/unbuffer(), which we can be fixed with a brief synchronized checkOpen() + snapshot before createChunkInputStream, without synchronizing the whole pread
I will address it in next revision.
Tried to implement HDDS-16325 but the BlockInputStream related classes are far from being able to support concurrent access. Filed also HDDS-16335. I guess this will need more subtasks. |
|
@taklwu what is the use case in Hbase for concurrent seek/read access to InputStream from multiple threads in Hbase, it seems like an error-prone implementation, lets assume that 2 threads are doing:
Thread 2
if they perform both perform operations without synchronizations between threads it can lead to the following execution sequence:
And both threads would read data at the wrong offset. And its true for any client. So if client does internal synchronization between readers' treads, InputThread itself can remain non-thread safe. |
yandrey321
left a comment
There was a problem hiding this comment.
Do we really need to support concurrent read/seek from multiple threads? and what would be the guarantees from the InputStream perspective?
taklwu
left a comment
There was a problem hiding this comment.
thanks @szetszwo , I'm proposing to fix it in readChunkAt with below , and that should address your concern
private int readChunkAt(ChunkInfo chunkInfo, long chunkOffset, int numBytesToRead, ByteBuffer dst)
throws IOException {
final int startPosition = dst.position();
int preadRetries = 0;
while (true) {
final ChunkInputStream chunkStream;
synchronized (this) {
checkOpen();
chunkStream = createChunkInputStream(chunkInfo);
}
| final long[] offsets = chunkOffsets; | ||
| final BlockData currentBlockData = blockData; | ||
| final long blockLength = length; |
There was a problem hiding this comment.
sorry, may ask why it need to be synchronized? I thought chunkOffsets, blockData and length were being initialized once in the initialize() which is a synchronized function already, here the readPositioned implemented as stateless and is trying to getting the the snapshot of this read only information for further operation within readPositioned
maybe I missed something that these three data will be changed over time?
| final int startPosition = dst.position(); | ||
| int preadRetries = 0; | ||
| while (true) { | ||
| final ChunkInputStream chunkStream = createChunkInputStream(chunkInfo); |
There was a problem hiding this comment.
I rechecked with below, so it seems the synchronized should not be required in the body
gRPC path (blockFileInputStream == null):
- The ephemeral ChunkInputStream gets its own xceiverClient via ChunkInputStream.acquireClient() (synchronized per instance).
- That uses xceiverClientFactory.acquireClientForReadData(), which is thread-safe internally (XceiverClientManager synchronizes on its cache).
- Block-level xceiverClientGrpc / xceiverClientShortCircuit are only used for getBlockData() during init, not for pread data reads.
Short-circuit path:
- LocalChunkInputStream uses positional FileChannel.read(buffer, pos) so concurrent preads on the shared block channel are safe.
- Its acquireClient() is a no-op; it does not use the block’s short-circuit client for reads
but I did checked that the real edge case is pread vs close()/unbuffer(), which we can be fixed with a brief synchronized checkOpen() + snapshot before createChunkInputStream, without synchronizing the whole pread
I will address it in next revision.
yeah, you found the problem of what HBase has been using Hadoop's
I don't have good judgement on this, but HBase is not the only one ask for this feature, you can see use case in HDDS-15734 also want this thread-safe positional read. So it depends on how much we want to support the existing HDFS's DFSInputStream use cases. |
is it possible to keep seek and read non-thread safe and optimized for non-concurrent usage; and guarantee thread safety for positional read APIs for concurrent usage? |
@taklwu , We are copying chunkOffsets, blockData and length there but some of them may mutate during copying. |
I thought we're already doing that in this PR , e.g. for the positioned read APIs aka org.apache.hadoop.fs.PositionedReadable (byte-array flavour)
org.apache.hadoop.fs.ByteBufferPositionedReadable (ByteBuffer flavour)
we didn't touch the seek (e.g. Mainly we make the existing
|
|
@taklwu , Both XceiverClientGrpc and XceiverClientShortCircuit are not threadsafe. In this PR, xceiverClient is not synchronized. The following is the trace: ChunkInputStream.doPositionedRead //new code in this PR |
|
@szetszwo I made a change to make sure the |
This sounds like a good idea! Let's separate the change to a new JIRA for refactoring the code. It will be easier to review. |
@szetszwo sorry I didn't express the statement clearly. I actually made that change and included as part of the latest revision. Can you review and see if that address your concerns? if not , we may need to split to another JIRA. additionally, I split this code with two more followup that learnt form Sergey's path2 patches.
|
szetszwo
left a comment
There was a problem hiding this comment.
@taklwu , thanks for the update!
- You are right that xceiverClient looks good now.
- This change is quite big. Let's separate the OzoneCryptoInputStream and OzoneFSInputStream changes to a different JIRA. (I have not reviewed them.)
Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13084411/11102_review.patch
| private int partIndexForPosition(long pos) { | ||
| int idx = Arrays.binarySearch(partOffsets, pos); | ||
| if (idx < 0) { | ||
| // binarySearch returns -insertionPoint - 1; the containing part is | ||
| // insertionPoint - 1. | ||
| idx = -idx - 2; | ||
| } | ||
| return idx; | ||
| } |
There was a problem hiding this comment.
Let's make it static and reuse it in BlockInputStream:
static int binarySearchOffsetIndex(long[] offsets, long pos) {
final int idx = Arrays.binarySearch(offsets, pos);
if (idx > 0) {
return idx;
}
// binarySearch returns n = -insertionPoint - 1;
// insertionPoint is -n - 1
// the containing index is insertionPoint - 1.
return -idx - 2;
}
I removed the changes of |
-align with HDDS-15920
- use positional FileChannel reads for local read - fix retry with the same token when using block input stream - remove the opt-in flag, default to stateless pread for ExtendedInputStream and fallback to sync if it's all other input stream
…putStream. - Use ephemeral ChunkInputStreams with per-call retry counters for block pread so concurrent positioned reads do not share sequential retry state or chunk stream buffers. - Serialize cursor-moving and positioned reads on OzoneCryptoInputStream because CryptoInputStream is not thread-safe.
- we need the protected readChunk in ChunkInputStream that used by LocalChunkInputStream and DummyChunkInputStream, other we will see NPE during execution. - fix unit test after HDDS-16100 merged
| * LocalChunkInputStream reads from a local FileChannel; no xceiver client is needed. | ||
| */ | ||
| @Override | ||
| protected synchronized XceiverClientSpi acquireClient() { | ||
| return null; | ||
| } |
| @Override | ||
| protected ByteBuffer[] readChunk(ChunkInfo readChunkInfo, XceiverClientSpi client) throws IOException { | ||
| return readChunk(readChunkInfo); | ||
| } |
There was a problem hiding this comment.
this is needed otherwise the datanodeBlockID is emitting NPE
|
all below failures are not related, I may trigger another execution.
|
|
@szetszwo it's ready for another look |
Overriding the acquireClient/readChunk makes code confusing; see below for my suggestion
So, I suggest to acquire a client directly from the factory and release it after use: int readPositioned(long chunkRelativePosition, ByteBuffer dst) throws IOException {
if (chunkRelativePosition < 0 || chunkRelativePosition >= length) {
return EOF;
}
final int toRead = (int) Math.min(dst.remaining(), length - chunkRelativePosition);
if (toRead == 0) {
return 0;
}
final ChunkInfo readChunkInfo = getChunkInfo(chunkRelativePosition, toRead);
final long adjustedOffset = readChunkInfo.getOffset() - chunkInfo.getOffset();
final long skip = chunkRelativePosition - adjustedOffset;
if (xceiverClientFactory == null) {
return copyRange(readChunk(readChunkInfo), skip, toRead, dst);
}
final Pipeline pipeline = pipelineSupplier.get();
final ContainerProtos.DatanodeBlockID bid = buildDatanodeBlockId(pipeline, blockID);
final XceiverClientSpi client = xceiverClientFactory.acquireClientForReadData(pipeline);
try {
final ByteBuffer[] readBuffers = readChunk(client, readChunkInfo, bid, validators, tokenSupplier.get());
return copyRange(readBuffers, skip, toRead, dst);
} finally {
xceiverClientFactory.releaseClientForReadData(client, false);
}
} private static ByteBuffer[] readChunk(
XceiverClientSpi client, ChunkInfo chunk, DatanodeBlockID blockID,
List<Validator> validators, Token<? extends TokenIdentifier> token) throws IOException {
Objects.requireNonNull(client, "client");
final ReadChunkResponseProto readChunkResponse = ContainerProtocolCalls.readChunk(
client, chunk, blockID, validators, token);
if (readChunkResponse.hasData()) {
return readChunkResponse.getData().asReadOnlyByteBufferList()
.toArray(new ByteBuffer[0]);
} else if (readChunkResponse.hasDataBuffers()) {
List<ByteString> buffersList = readChunkResponse.getDataBuffers()
.getBuffersList();
return BufferUtils.getReadOnlyByteBuffersArray(buffersList);
} else {
throw new IOException("Unexpected error while reading chunk data " +
"from container. No data returned.");
}
} |
What changes were proposed in this pull request?
Fix Non-Stream read failed positional read checksum intermittently during concurrently access by multi-threads.
Please describe your PR in detail:
Fixing the concurrent positional read by introducing stateless pread for major input streams (BlockInputStream, ChunkInputStream, MultiPartInputStream) as the default pread method, such that the read could be getting the range / chunkinfo via readChunk (additional RPC) and use
copyRangewithout touching shared cursor / position and avoid race condition. any child class of ExtendedInputStream is now supported with stateless pread.The problem without thread-safe inputstream, checksum may fail because the offset has moved from the current position.
note that hadoop's FSInputStream does have this
synchronizedblock and make sure the input stream is thread-safe. Also we're learning from DFSInputStream's getBlockRange and have this logic as an additional RPC that used by readChunk and copyRange.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15424
How was this patch tested?