HADOOP-19613. [ABFS][ReadAheadV2] Refactor ReadBufferManager to isolate new code with the current working code #7801

anujmodi2021 · 2025-07-14T03:02:57Z

This is the first PR in series of work done under Parent Jira: HADOOP-19596 to improve the performance of sequential reads in ABFS Driver.
Please refer to Parent JIRA for more details.

Description of PR

Jira: https://issues.apache.org/jira/browse/HADOOP-19613

Read Buffer Manager used today was introduced way back and has been stable for quite a while.
Read Buffer Manager to be introduced as part of HADOOP-19596 will introduce many changes incrementally over time. While the development goes on and we are able to fully stabilise the optimized version we need the current flow to be functional and undisturbed.

This work item is to isolate that from new code by refactoring ReadBufferManager class to have 2 different implementations with same public interfaces: ReadBufferManagerV1 and ReadBufferManagerV2.

This will also introduce new configs that can be used to toggle between new and old code.

How was this patch tested?

Existing tests were modified to work with the Refactored Classes.
More tests will be added with coming up PRs where new implementation will be introduced.
Test suite result added.

anujmodi2021 · 2025-07-15T05:26:43Z

============================================================
HNS-OAuth-DFS

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 3
[WARNING] Tests run: 818, Failures: 0, Errors: 0, Skipped: 165
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 23

============================================================
HNS-SharedKey-DFS

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 821, Failures: 0, Errors: 0, Skipped: 117
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 10

============================================================
NonHNS-SharedKey-DFS

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 660, Failures: 0, Errors: 0, Skipped: 223
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 11

============================================================
AppendBlob-HNS-OAuth-DFS

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 3
[WARNING] Tests run: 818, Failures: 0, Errors: 0, Skipped: 176
[WARNING] Tests run: 133, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 23

============================================================
NonHNS-SharedKey-Blob

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 664, Failures: 0, Errors: 0, Skipped: 134
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 5
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 11

============================================================
NonHNS-OAuth-DFS

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 657, Failures: 0, Errors: 0, Skipped: 225
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 24

============================================================
NonHNS-OAuth-Blob

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 661, Failures: 0, Errors: 0, Skipped: 147
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 5
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 24

============================================================
AppendBlob-NonHNS-OAuth-Blob

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 659, Failures: 0, Errors: 0, Skipped: 165
[WARNING] Tests run: 133, Failures: 0, Errors: 0, Skipped: 6
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 24

============================================================
HNS-Oauth-DFS-IngressBlob

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 3
[WARNING] Tests run: 692, Failures: 0, Errors: 0, Skipped: 172
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 23

============================================================
NonHNS-OAuth-DFS-IngressBlob

[WARNING] Tests run: 177, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 657, Failures: 0, Errors: 0, Skipped: 223
[WARNING] Tests run: 156, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 269, Failures: 0, Errors: 0, Skipped: 24

bhattmanish98 · 2025-07-17T09:52:39Z

...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java

+   * Enable or disable readahead V2 in AbfsInputStream. This will work independent of V1.
+   * Value: {@value}.
+   */
+  public static final String FS_AZURE_ENABLE_READAHEAD_V2 = "fs.azure.enable.readahead.v2";


If both V1 and V2 are enabled, does V2 take precedence over V1? If so, would it be inaccurate to refer to them as independent?

By independent I mean, users don't have to explicitly enable fs.azure.enable.readahead for fs.azure.enable.readahead.v2 to work.

And yes if both are set, preference will be given to V2.

Can you add this precedence thing in the comments just for better visibility?

Already Added in AbfsInputStream where this precedence is actually taken into account.

bhattmanish98 · 2025-07-17T09:56:43Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java

  @VisibleForTesting
  public boolean isReadAheadEnabled() {
-    return readAheadEnabled;
+    return (readAheadEnabled || readAheadV2Enabled) && readBufferManager != null;


The method name suggests we're only checking readAhead, but we're also checking readBufferManager here. Should we either move this check outside the method or update the method name to reflect its functionality?

method is to check if readahaead is enabled or not and for that we need readBufferManager to be not null.
This is added only as a precaution to avoid NPE

anmolanmol1234 · 2025-07-18T08:10:46Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManager.java

-    return lengthToCopy;
-  }
+  @VisibleForTesting
+  void testResetReadBufferManager();


why does the method name in production have test ?

It was there already in RBMV1. It makes sense to keep it this way because this should never be called from production flow and its critical to test some really important scenarios so we can't void it.

anmolanmol1234 · 2025-07-18T08:19:18Z

...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV1.java

+  private static ReadBufferManagerV1 bufferManager; // singleton, initialized in static initialization block
+  private static final ReentrantLock LOCK = new ReentrantLock();
+
+  static ReadBufferManagerV1 getBufferManager() {


since we are refactoring the code, we can add javadocs for all the methods as well

Makes sense. Will take it

anmolanmol1234 · 2025-07-18T08:24:39Z

...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV1.java

+   *
+   * @param stream          The {@link AbfsInputStream} for which to do the read-ahead
+   * @param requestedOffset The offset in the file which shoukd be read
+   * @param requestedLength The length to read


tracing context missing in params

anmolanmol1234 · 2025-07-18T10:23:26Z

...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV1.java

+  public ReadBuffer getNextBlockToRead() throws InterruptedException {
+    ReadBuffer buffer = null;
+    synchronized (this) {
+      //buffer = readAheadQueue.take();  // blocking method


remove comments

manika137 · 2025-07-21T02:53:00Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBuffer.java

@@ -26,7 +26,7 @@

 import static org.apache.hadoop.fs.azurebfs.contracts.services.ReadBufferStatus.READ_FAILED;

-class ReadBuffer {
+public class ReadBuffer {


why did we make it public? The new classes seem to be under the same package as this

manika137 · 2025-07-21T03:00:12Z

...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV1.java

+   * @param position the offset in the file to do a read for
+   * @param length   the length to read
+   * @param buffer   the buffer to read data into. Note that the buffer will be written into from offset 0.
+   * @return the number of bytes read


nit: we can add the @throws IOException line

Added in base class

manika137 · 2025-07-21T03:02:21Z

...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV1.java

+   * Returns buffers that failed or passed from completed queue.
+   * @param stream
+   * @param requestedOffset
+   * @return


nit: missing @return ReadBuffer

Added in base class

manika137 · 2025-07-21T03:20:47Z

...ools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferWorker.java

@@ -51,7 +51,7 @@ public void run() {
    } catch (InterruptedException ex) {
      Thread.currentThread().interrupt();
    }
-    ReadBufferManager bufferManager = ReadBufferManager.getBufferManager();
+    ReadBufferManager bufferManager = ReadBufferManagerV1.getBufferManager();


will this always require ReadBufferManagerV1?

No, this will be change in next PR when ReadBufferManagerV2 will be introduced.

manika137 · 2025-07-21T03:25:54Z

...s/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsInputStream.java

+
+  private ReadBufferManager getBufferManager() {
+    return ReadBufferManagerV1.getBufferManager();
+  }
 }


nit: add EOL

hadoop-yetus · 2025-07-22T20:16:09Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 34s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 2 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	40m 3s		trunk passed
+1 💚	compile	0m 42s		trunk passed with JDK Ubuntu-11.0.27+6-post-Ubuntu-0ubuntu120.04
+1 💚	compile	0m 38s		trunk passed with JDK Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	checkstyle	0m 34s		trunk passed
+1 💚	mvnsite	0m 44s		trunk passed
+1 💚	javadoc	0m 44s		trunk passed with JDK Ubuntu-11.0.27+6-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	0m 36s		trunk passed with JDK Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	spotbugs	1m 10s		trunk passed
+1 💚	shadedclient	35m 26s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	35m 48s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 32s		the patch passed
+1 💚	compile	0m 32s		the patch passed with JDK Ubuntu-11.0.27+6-post-Ubuntu-0ubuntu120.04
+1 💚	javac	0m 32s		the patch passed
+1 💚	compile	0m 29s		the patch passed with JDK Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	javac	0m 29s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 23s	/results-checkstyle-hadoop-tools_hadoop-azure.txt	hadoop-tools/hadoop-azure: The patch generated 27 new + 3 unchanged - 0 fixed = 30 total (was 3)
+1 💚	mvnsite	0m 33s		the patch passed
+1 💚	javadoc	0m 29s		the patch passed with JDK Ubuntu-11.0.27+6-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	0m 28s		the patch passed with JDK Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
+1 💚	spotbugs	1m 8s		the patch passed
+1 💚	shadedclient	35m 14s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	3m 8s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 40s		The patch does not generate ASF License warnings.
		126m 7s

Subsystem	Report/Notes
Docker	ClientAPI=1.51 ServerAPI=1.51 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7801/8/artifact/out/Dockerfile
GITHUB PR	#7801
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 876cdedb2eb7 5.15.0-139-generic #149-Ubuntu SMP Fri Apr 11 22:06:13 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `5b93b32`
Default Java	Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.27+6-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_452-8u452-ga~~us1-0ubuntu1~~20.04-b09
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7801/8/testReport/
Max. process+thread count	707 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7801/8/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

anujmodi2021 added 2 commits July 13, 2025 05:49

Refactoring RBM

fa214ea

Adding Configurations for RBM

c1a6fd0

github-actions bot added trunk TOOLS ABFS labels Jul 14, 2025

PR Checks

fc9324d

anujmodi2021 marked this pull request as ready for review July 14, 2025 06:11

anujmodi2021 self-assigned this Jul 15, 2025

bhattmanish98 reviewed Jul 17, 2025

View reviewed changes

anmolanmol1234 reviewed Jul 18, 2025

View reviewed changes

manika137 reviewed Jul 21, 2025

View reviewed changes

anujmodi2021 added 3 commits July 21, 2025 11:18

Added Jaavdocs

ccae40d

PR Checks

99fb6a9

PR Checks

643d30c

This comment was marked as outdated.

Sign in to view

Yetus Checks

0bfbfb9

This comment was marked as outdated.

Sign in to view

anujmodi2021 added 3 commits July 22, 2025 04:18

Yetus Checks

3dda35d

Making ReadBufferManager Abstract

727bd51

Fixing test

9ce4f81

This comment was marked as outdated.

Sign in to view

ReadBufferManagerV2

2019935

This comment was marked as outdated.

Sign in to view

Yetus Checks

5b93b32

HADOOP-19613. [ABFS][ReadAheadV2] Refactor ReadBufferManager to isolate new code with the current working code #7801

Are you sure you want to change the base?

HADOOP-19613. [ABFS][ReadAheadV2] Refactor ReadBufferManager to isolate new code with the current working code #7801

Conversation

anujmodi2021 commented Jul 14, 2025

Description of PR

How was this patch tested?

Uh oh!

anujmodi2021 commented Jul 15, 2025

============================================================ HNS-OAuth-DFS

============================================================ HNS-SharedKey-DFS

============================================================ NonHNS-SharedKey-DFS

============================================================ AppendBlob-HNS-OAuth-DFS

============================================================ NonHNS-SharedKey-Blob

============================================================ NonHNS-OAuth-DFS

============================================================ NonHNS-OAuth-Blob

============================================================ AppendBlob-NonHNS-OAuth-Blob

============================================================ HNS-Oauth-DFS-IngressBlob

============================================================ NonHNS-OAuth-DFS-IngressBlob

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

hadoop-yetus commented Jul 22, 2025

Uh oh!

Uh oh!

============================================================
HNS-OAuth-DFS

============================================================
HNS-SharedKey-DFS

============================================================
NonHNS-SharedKey-DFS

============================================================
AppendBlob-HNS-OAuth-DFS

============================================================
NonHNS-SharedKey-Blob

============================================================
NonHNS-OAuth-DFS

============================================================
NonHNS-OAuth-Blob

============================================================
AppendBlob-NonHNS-OAuth-Blob

============================================================
HNS-Oauth-DFS-IngressBlob

============================================================
NonHNS-OAuth-DFS-IngressBlob