Copilot commented on code in PR #11102:
URL: https://github.com/apache/ozone/pull/11102#discussion_r3848084210
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/MultipartInputStream.java:
##########
@@ -219,6 +228,57 @@ int readImpl(InputStream inputStream) throws IOException {
return true;
}
+ /**
+ * Stateless positioned read for the replicated (Ratis) path. Routes the read
+ * across the part {@link BlockInputStream}s using the immutable
+ * {@link #partOffsets} without seeking or mutating the shared cursor, so
+ * concurrent positioned reads run independently. Returns {@code false} (so
+ * the caller can fall back) when any part is not a {@link BlockInputStream},
+ * e.g. erasure coded parts.
+ */
+ private boolean readFullyStateless(long position, ByteBuffer buffer)
+ throws IOException {
+ if (!buffer.hasRemaining()) {
+ return true;
+ }
+ if (partStreams.isEmpty()) {
+ return false;
+ }
+ for (PartInputStream part : partStreams) {
+ if (!(part instanceof BlockInputStream)) {
+ return false;
+ }
+ }
Review Comment:
readFullyStateless() performs an O(partStreams) type-check on every
positioned read. For large keys with many parts, this adds overhead on the hot
pread path; the result could be computed once in the constructor (partStreams
is immutable) and reused.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:
##########
@@ -180,6 +195,19 @@ public int read(long position, ByteBuffer buf) throws
IOException {
}
}
+ // Fallback: stateful seek-read-restore on the shared cursor. This is only
+ // thread-safe when enabled. The lock is only needed for the
seek-read-restore sequence, so it is
+ // not held for the entire read operation.
Review Comment:
This fallback comment is both misleading (the lock actually covers the
entire seek/read/restore sequence) and exceeds the 120 character checkstyle
limit on the last line.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -486,6 +487,52 @@ protected synchronized int
readWithStrategy(ByteReaderStrategy strategy)
* 2. chunkStream[2] will be seeked to position 10
* (= 90 - chunkOffset[2] (= 80)).
*/
+ /**
+ * Stateless positioned read across this block's chunks. Fills up to
+ * {@code dst.remaining()} bytes starting from {@code blockRelativePosition}
+ * without mutating this stream's cursor ({@code chunkIndex},
+ * {@code blockPosition}) or the chunk streams' buffered state, so it is safe
+ * for concurrent callers. Metadata ({@code chunkOffsets}, {@code
+ * chunkStreams}, {@code length}) is published once by {@link #initialize()}.
+ *
+ * @return bytes copied into {@code dst}, or {@link #EOF} at EOF
+ */
+ int readPositioned(long blockRelativePosition, ByteBuffer dst)
+ throws IOException {
+ if (!initialized) {
+ initialize();
+ }
Review Comment:
readPositioned() is intended for concurrent callers, but it reads
initialization-published fields (initialized/chunkStreams/chunkOffsets/length)
without any happens-before edge. Since initialized is not volatile and this
method is not synchronized, another thread can observe initialized==true while
still seeing stale/null chunkStreams/chunkOffsets, leading to races/incorrect
reads/NPEs under concurrent pread.
##########
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/DummyChunkInputStream.java:
##########
@@ -64,13 +64,16 @@ protected ByteBuffer[] readChunk(ChunkInfo readChunkInfo) {
ByteString byteString = ByteString.copyFrom(chunkData,
offset, bufferLen);
- readByteBuffers.add(byteString);
+ chunkBuffers.add(byteString);
offset += bufferLen;
remainingToRead -= bufferLen;
}
- return BufferUtils.getReadOnlyByteBuffers(readByteBuffers)
+ readByteBuffers.clear();
+ readByteBuffers.addAll(chunkBuffers);
+
+ return BufferUtils.getReadOnlyByteBuffers(chunkBuffers)
.toArray(new ByteBuffer[0]);
}
Review Comment:
DummyChunkInputStream.readChunk() now participates in concurrent
positioned-read tests, but it mutates the shared ArrayList readByteBuffers via
clear()/addAll() without synchronization. Concurrent readChunk() calls can
corrupt the list or throw runtime exceptions, making the new concurrency tests
flaky.
--
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]
