HDDS-16400. Fix thread-safety of positioned reads in OzoneFSInputStream by taklwu · Pull Request #11245 · apache/ozone

taklwu · 2026-09-15T20:22:19Z

What changes were proposed in this pull request?

followup by HDDS-15424, we have created HDDS-16400 that implements the thread-safe positional read for OzoneFSInputStream

Please describe your PR in detail:

ECBlockInputStream#readFully return false (use the default ExtendedInputStream#readFully), so it fail back to use the new additional synchronized lock for readSeekRestore.
for those non-ByteBuffer / non-ByteBufferReadable inputstream, we have to use byte[] for seekReadRestore instead of casting with ByteBufferReadable

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16400

How was this patch tested?

unit tests.

Copilot

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Improves thread safety for positional reads through Ozone filesystem and encrypted streams.

Changes:

Synchronizes fallback seek/read/restore operations.
Adds synchronized positional APIs to OzoneCryptoInputStream.
Adds concurrent positioned-read tests and the required test dependency.

File summaries

File	Description
hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSInputStream.java	Updated as part of this pull request.
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java	Updated as part of this pull request.
hadoop-ozone/ozonefs-common/pom.xml	Updated as part of this pull request.
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java	Updated as part of this pull request.

Review details

Suppressed comments (2)

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java:84

The synchronization added here does not cover inherited cursor-mutating operations such as skip. A concurrent skip can change CryptoInputStream's cursor or internal buffer while read(long, ByteBuffer) is between its seek/read/restore steps, so the positioned read can return corrupted data despite the new thread-safety guarantee. Override the cursor-mutating methods that are not already synchronized (at least skip), or narrow this documentation and the guarantee to the synchronized operations.

   * {@link CryptoInputStream} does not synchronize its own methods, so every method moving the cursor of
   * this stream is serialized here on the monitor of this stream. Otherwise a read or a seek could land in
   * the middle of a positioned read and see (or undo) the cursor move that read does.

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:42

MultipartInputStream.readFully is not always stateless: its StreamBlockInputStream path performs a synchronized seek/read/restore sequence (see MultipartInputStream.java:197-233), while only the block path is stateless. Describing every successful ExtendedInputStream path as stateless is misleading for the concurrency guarantee; please call this the underlying positioned-read implementation or distinguish the two modes.

 * 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.

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🔵 Needs a closer look

Validate read-only ByteBuffer destinations before positioned reads to prevent state corruption after an exception.

Review details

Suppressed comments (1)

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java:183

A read-only destination is not rejected before the seek/read sequence. For an unaligned or short request, read(byte[], ...) sets readPositionAdjustedBy/readLengthAdjustedBy and then dst.put(...) throws ReadOnlyBufferException; the finally restores only the cursor, leaving those adjustment fields set, so the next valid read fails the precondition at getNumBytesToRead. Check dst.isReadOnly() before starting the positioned read (as the other ByteBuffer read path does).

  public synchronized int read(long position, ByteBuffer dst) throws IOException {
    if (!dst.hasRemaining()) {
      return 0;

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟡 Changes recommended

Unresolved cursor-safety and byte-array positioned-read compatibility issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Lite

Copilot

🟡 Changes recommended

The byte-array overrides regress validation, EOF handling, and read statistics.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java:258

This reimplementation loses two guarantees of the inherited readFully: it skips argument validation on the non-ExtendedInputStream path, and it treats ExtendedInputStream.readFully(...) == true as proof that the buffer is full. MultipartInputStream can return true after only reading the suffix available before EOF, so this silently leaves the array tail untouched instead of throwing. Now that the overridden read(...) uses the shared lock, delegate to the inherited validating loop.

  public void readFully(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)) {

Files reviewed: 3/3 changed files
Comments generated: 3
Review effort level: Balanced

taklwu · 2026-09-16T18:31:08Z

@szetszwo This is the first split PR for OzoneFSInputStream, focusing mainly on input streams that do not implement ExtendedInputStream (aka EC). meanwhile we will have OzoneCryptoInputStream support thread-safe positional read in next PR.

szetszwo

@taklwu , thanks for working on this! Please see the comments inlined.

szetszwo · 2026-09-19T16:40:01Z

+   * 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 {


For byte array read and readFully, let's just call the ByteBuffer methods?

@Override public int read(long position, byte[] buffer, int offset, int length) throws IOException { validatePositionedReadArgs(position, buffer, offset, length); return read(position, ByteBuffer.wrap(buffer, offset, length)); } @Override public void readFully(long position, byte[] buffer, int offset, int length) throws IOException { validatePositionedReadArgs(position, buffer, offset, length); readFully(position, ByteBuffer.wrap(buffer, offset, length)); }

I mentioned that in the comment that if the the inner input stream is not child class of ExtendedInputStream, we will cannot use ByteBuffer.wrap because not all Seekable streams also implement ByteBufferReadable, and the casting may fail line#201 with ((ByteBufferReadable) inputStream).read(buf); .

but if you think we only support ByteBufferReadable , then we can reuse those existing method.

... we only support ByteBufferReadable , ...

In current code, are the inner input streams always ByteBufferReadable? If yes, let's only support ByteBufferReadable. (We may add a assertion in the constructor as well.)

for GDPRSymmetricKey aka Non-encrypted GDPR inputstream in RpcClient line 2680, RpcClient.createInputStream -> new OzoneInputStream( new CipherInputStream(lengthInputStream, gk.getCipher())) where CipherInputStream does not support ByteBufferReadable

so, should we keep the readAtPositionSeekRestoreByteArray?

I see. Then, we should implement the array version.

Let's make the array and ByteBuffer version look similar.

We should also create private methods and reuse the code; see https://issues.apache.org/jira/secure/attachment/13084654/11245_review.patch

Co-authored-by: Cursor <cursoragent@cursor.com>

szetszwo · 2026-09-21T19:06:31Z

taklwu force-pushed the HDDS-16400 branch from f64eb71 to 8f6617b ...

@taklwu , Please don't force pushed since we cannot tell what are the new changes -- it also forces the reviewer to read everything from the beginning.

taklwu · 2026-09-21T21:53:00Z

ah, sorry, I normally rebase and push everything, I will just push without rebase next time.

FYI the new changes are only the last commit 8f6617b

szetszwo

@taklwu , thanks for the update! Please see the comments inlined.

szetszwo · 2026-09-22T04:07:21Z

+   * 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 {


I see. Then, we should implement the array version.

Let's make the array and ByteBuffer version look similar.

We should also create private methods and reuse the code; see https://issues.apache.org/jira/secure/attachment/13084654/11245_review.patch

taklwu · 2026-09-22T15:51:27Z

TestOzoneManagerHASnapshot.testSnapshotDiffWhenOmLeaderRestart failure is not related.

[ERROR] org.apache.hadoop.ozone.om.snapshot.TestOzoneManagerHASnapshot.testSnapshotDiffWhenOmLeaderRestart -- Time elapsed: 8.106 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <DONE> but was: <IN_PROGRESS>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)

szetszwo

+1 the change looks good.

adoroszlai · 2026-09-22T17:49:35Z

Thanks @taklwu for the patch, @szetszwo for the review.

Copilot AI lite review requested due to automatic review settings September 15, 2026 20:22

Copilot started reviewing on behalf of taklwu September 15, 2026 20:22 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java Outdated

Comment thread hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java Outdated

taklwu force-pushed the HDDS-16400 branch from f55e4a3 to 358c1cf Compare September 15, 2026 20:57

taklwu changed the title ~~HDDS-16400. Make positional read thread-safe for OzoneCryptoInputStre…~~ HDDS-16400. Support concurrent positioned reads in OzoneFSInputStream and OzoneCryptoInputStream Sep 15, 2026

taklwu requested a lite review from Copilot September 15, 2026 20:58

Copilot started reviewing on behalf of taklwu September 15, 2026 20:58 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

taklwu force-pushed the HDDS-16400 branch from 358c1cf to 9a11224 Compare September 15, 2026 21:36

taklwu requested a lite review from Copilot September 15, 2026 21:38

Copilot started reviewing on behalf of taklwu September 15, 2026 21:39 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneCryptoInputStream.java Outdated

Comment thread hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFSInputStream.java

taklwu force-pushed the HDDS-16400 branch from 9a11224 to b35981c Compare September 15, 2026 22:33

taklwu changed the title ~~HDDS-16400. Support concurrent positioned reads in OzoneFSInputStream and OzoneCryptoInputStream~~ HDDS-16400. Fix thread-safety of positioned reads in OzoneFSInputStream Sep 15, 2026

taklwu requested a balanced review from Copilot September 15, 2026 22:42

Copilot started reviewing on behalf of taklwu September 15, 2026 22:42 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

taklwu mentioned this pull request Sep 16, 2026

HDDS-16426. Fix thread-safety of positioned reads in OzoneCryptoInputStream #11251

Merged

szetszwo reviewed Sep 19, 2026

View reviewed changes

taklwu and others added 4 commits September 21, 2026 11:18

HDDS-16400. Fix thread-safety of positioned reads in OzoneFSInputStream

67025cd

Co-authored-by: Cursor <cursoragent@cursor.com>

address comment and align with AbstractOzoneContractTest

bf652d3

fix another unit test for position<=-1

6b7f210

address comments

8f6617b

taklwu force-pushed the HDDS-16400 branch from f64eb71 to 8f6617b Compare September 21, 2026 18:41

szetszwo reviewed Sep 22, 2026

View reviewed changes

refactor the read functions and tests

f39a41b

szetszwo approved these changes Sep 22, 2026

View reviewed changes

adoroszlai merged commit f73fa1a into apache:master Sep 22, 2026
88 of 89 checks passed

taklwu deleted the HDDS-16400 branch September 22, 2026 18:37

Conversation

taklwu commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taklwu commented Sep 16, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szetszwo Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

taklwu Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

szetszwo Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

taklwu Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

szetszwo Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

szetszwo commented Sep 21, 2026

Uh oh!

taklwu commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

szetszwo Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

taklwu commented Sep 22, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

taklwu commented Sep 15, 2026 •

edited

Loading

taklwu commented Sep 21, 2026 •

edited

Loading