Copilot commented on code in PR #11245:
URL: https://github.com/apache/ozone/pull/11245#discussion_r4020867234
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:
##########
@@ -211,4 +222,75 @@ public void readFully(long position, ByteBuffer buf)
throws IOException {
}
}
}
+
+ /**
+ * Byte-array positioned read. Tries the native stateless {@link
ExtendedInputStream#readFully}
+ * path first (via a zero-copy {@link ByteBuffer#wrap}). Falls back to a
synchronized
+ * seek-read-restore using byte-array {@link #read(byte[], int, int)} so
that the fallback
+ * works for any {@link Seekable} stream, not just those that also implement
+ * {@link org.apache.hadoop.fs.ByteBufferReadable}.
+ * <p>
+ * {@link FSInputStream} synchronizes its inherited implementation on {@code
this}, a different
+ * monitor from {@code positionedReadLock}; without this override the two
APIs can interleave.
+ */
+ @Override
+ public int read(long position, byte[] buffer, int offset, int length) throws
IOException {
+ if (inputStream instanceof ExtendedInputStream) {
Review Comment:
This override bypasses `FSInputStream.validatePositionedReadArgs` on the
fallback path. Consequently invalid slices can mutate the cursor before
failing, and a zero-length read can return `-1` at EOF if the wrapped stream
does so (the new `SeekableOnlyInputStream` has exactly that behavior). Preserve
the inherited validation and empty-read contract before selecting a path.
This issue also appears on line 254 of the same file.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:
##########
@@ -211,4 +222,75 @@ public void readFully(long position, ByteBuffer buf)
throws IOException {
}
}
}
+
+ /**
+ * Byte-array positioned read. Tries the native stateless {@link
ExtendedInputStream#readFully}
+ * path first (via a zero-copy {@link ByteBuffer#wrap}). Falls back to a
synchronized
+ * seek-read-restore using byte-array {@link #read(byte[], int, int)} so
that the fallback
+ * works for any {@link Seekable} stream, not just those that also implement
+ * {@link org.apache.hadoop.fs.ByteBufferReadable}.
+ * <p>
+ * {@link FSInputStream} synchronizes its inherited implementation on {@code
this}, a different
+ * monitor from {@code positionedReadLock}; without this override the two
APIs can interleave.
+ */
+ @Override
+ public int read(long position, byte[] buffer, int offset, int length) throws
IOException {
+ if (inputStream instanceof ExtendedInputStream) {
+ final ByteBuffer buf = ByteBuffer.wrap(buffer, offset, length);
+ try {
+ if (((ExtendedInputStream) inputStream).readFully(position, buf)) {
+ return length - buf.remaining();
+ }
Review Comment:
The native branch bypasses this wrapper's byte-read accounting. Before this
override, `FSInputStream.read(position, ...)` called the virtual `read(byte[],
...)`, which increments `FileSystem.Statistics`; now successful replicated
positioned reads disappear from `bytesRead`. Increment the statistic by the
buffer advancement before returning.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:
##########
@@ -37,7 +37,9 @@
* The input stream for Ozone file system.
*
* TODO: Make inputStream generic for both rest and rpc clients
- * This class is not thread safe.
+ * Sequential reads are not thread safe. Positioned reads use a native
+ * stateless path when the underlying {@link ExtendedInputStream} supports it;
+ * otherwise they fall back to a synchronized seek-read-restore sequence.
Review Comment:
This description is inaccurate for supported `ExtendedInputStream`
implementations: `MultipartInputStream.readFullyStreamBlock` reports support
while performing its own synchronized, stateful seek-read-restore. Describe
this as delegation to the underlying positioned-read implementation rather than
promising that every supported path is stateless.
--
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]
