HDDS-16323. StreamBlockInputStream: refill the pre-read window in bulk instead of once per response

[ss77892]

What changes were proposed in this pull request?

StreamBlockInputStream: refill the pre-read window in bulk instead of once per response

This change adds an early return in readBlock(): when the caller's read is already covered by what has been requested (required <= 0) and the outstanding pre-read window is still at least half full, no new request is sent. The window is refilled in bulk only once it drains below half. When required > 0 the request is always sent, so a read that needs bytes beyond the requested range is never starved.

A unit test (testPreReadWindowIsRefilledInBulk) reads a block one response at a time, as KeyInputStream does, and checks that the number of ReadBlock requests is bounded by the number of half windows and that every request carries at least half a window.

What is the link to the Apache JIRA

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

How was this patch tested?

UT added/ CI
Performance evaluation on 3 node cluster.
HDDS-16323.pdf

[chihsuan]

Thanks for the patch! @ss77892 +1, this looks good to me. 👍

[chihsuan]

nit: Just a thought, what do you think about giving the / 2 a name? A named constant might make the half-window trigger easier to spot later.

[ss77892]

Sure. I've added preReadRefillThreshold that is computed alongside preReadSize, so the test can reference the same value rather than repeating the / 2.

[chungen0126]

Thanks @ss77892 for working on this. Is the pre-read window configurable? I think we should also update the description for ozone.client.stream.read.pre-read-size in OzoneClientConfig.

[ss77892]

Thanks @ss77892 for working on this. Is the pre-read window configurable? I think we should also update the description for ozone.client.stream.read.pre-read-size in OzoneClientConfig.

@chungen0126 thank you for looking into it. Yes, it's configurable via ozone.client.stream.read.pre-read-size (32 MB by default). The refill threshold is simply half of that, so it follows whatever window size is set. I didn't add a separate key for it since I don't see a case where someone would need to tune it on its own.
Good point about the description, it was too vague. I've updated it to explain the window and when it gets refilled.

[ss77892]

@chihsuan @chungen0126 Thank you for looking into this PR. anything that I should also address?

[chungen0126]

+1 LGTM

[chungen0126]

@szetszwo Would you like to take a look?

[szetszwo]

Sure, let me take a look.

BTW, TestStreamReadDatanodeFailover has failed. It looks like related.

[szetszwo]

@ss77892 , thanks for working on this! The change looks good. Just has some minor suggestions to make the code easier to understand:

• remove the preReadRefillThreshold field.
• change getPreReadRefillThreshold() to static
• add an outstanding local variable in readBlock(..)

See below:

+++ b/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java
@@ -349,8 +349,13 @@ private synchronized void initialize() throws IOException {
   }
 
   synchronized void readBlock(int length, boolean preRead) throws IOException {
-    final long required = position + length - requestedLength;
+    final long outstanding = requestedLength - position;  // bytes already waiting in poll()
+    final long required = length - outstanding;           // bytes required to fulfill this call
     final long preReadLength = preRead ? preReadSize : 0;
+    if (required <= 0 && outstanding >= getPreReadRefillThreshold(preReadLength)) {
+      // Safe to skip: already has the required bytes and exceeded the refill threshold
+      return;
+    }
     // Clamp so requestedLength never exceeds blockLength: requesting past the end
     // produces an offset the DataNode cannot serve, causing a read timeout.
     final long readLength = Math.min(required + preReadLength, blockLength - requestedLength);
@@ -441,6 +446,10 @@ public long getPreReadSize() {
     return preReadSize;
   }
 
+  static long getPreReadRefillThreshold(long preRead) {
+    return preRead / 2 ;
+  }
+

[ss77892]

@ss77892 , thanks for working on this! The change looks good. Just has some minor suggestions to make the code easier to understand:

• remove the preReadRefillThreshold field.
• change getPreReadRefillThreshold() to static
• add an outstanding local variable in readBlock(..)

@szetszwo Thank you for checking it. I've committed the changes. The failure in CI is actually not related and reveals a bug introduced by combination of HDDS-15521 and HDDS-16207. I will address it in a separate jira.