HADOOP-19737: ABFS: Add metrics to identify improvements with read and write aggressiveness #8056
+1,871
−230
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Contributor
anujmodi2021
left a comment
anujmodi2021
left a comment
There was a problem hiding this comment.
Partial Review. Will complete in some time
...c/main/java/org/apache/hadoop/fs/azurebfs/enums/AbfsWriteResourceUtilizationMetricsEnum.java
Show resolved
Hide resolved
...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Private constructor to prevent instantiation as this needs to be singleton. | ||
| * Initializes a new instance of {@code ReadBufferManagerV2} for the given ABFS client. |
Contributor
There was a problem hiding this comment.
All the clients in JVM ae supposed to use same ReadBufferManager. This comment seems wrong
...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV2.java
Outdated
Show resolved
Hide resolved
...ools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java
Outdated
Show resolved
Hide resolved
...p-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsWriteThreadPoolMetrics.java
Outdated
Show resolved
Hide resolved
...adoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AzureDFSIngressHandler.java
Show resolved
Hide resolved
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.
This comment was marked as outdated.
This comment was marked as outdated.
anujmodi2021
requested changes
Nov 24, 2025
...rc/main/java/org/apache/hadoop/fs/azurebfs/enums/AbfsReadResourceUtilizationMetricsEnum.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingContext.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingContext.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
Outdated
Show resolved
Hide resolved
...op-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java
Outdated
Show resolved
Hide resolved
...s/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV2.java
Outdated
Show resolved
Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingHeaderVersion.java
Outdated
Show resolved
Hide resolved
| * Schema: version:clientCorrelationId:clientRequestId:fileSystemId | ||
| * :primaryRequestId:streamId:opType:retryHeader:ingressHandler | ||
| * :position:operatedBlobCount:operationSpecificHeader:httpOperationHeader | ||
| * :networkLibrary:operationMetrics |
Contributor
Author
There was a problem hiding this comment.
should be 16 with aggregated metrics and resource utilization metrics separate
Contributor
There was a problem hiding this comment.
Earlier it was 13 right?
We are only adding one more in this PR.
Contributor
Author
There was a problem hiding this comment.
Network library and aggregated metrics are not part of V1 as per javadoc, so 2 + the resourceUtilizationMetrics from this PR, hence made it 16
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/utils/TracingHeaderVersion.java
Outdated
Show resolved
Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/WriteThreadPoolSizeManager.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| public static final int DEFAULT_WRITE_HIGH_TIER_MEMORY_MULTIPLIER = 16; | ||
|
|
||
| /** Percentage threshold of heap usage at which memory pressure is considered high. */ | ||
| public static final int DEFAULT_WRITE_HIGH_MEMORY_USAGE_THRESHOLD_PERCENTAGE = 60; |
Contributor
There was a problem hiding this comment.
Since we have used PERCENT in the FileSystemConfiguration FS_AZURE_WRITE_HIGH_MEMORY_USAGE_THRESHOLD_PERCENT, should be better to rename this to DEFAULT_WRITE_HIGH_MEMORY_USAGE_THRESHOLD_PERCENT
| public static final int DEFAULT_WRITE_HIGH_MEMORY_USAGE_THRESHOLD_PERCENTAGE = 60; | ||
|
|
||
| /** Percentage threshold of heap usage at which memory pressure is considered low. */ | ||
| public static final int DEFAULT_WRITE_LOW_MEMORY_USAGE_THRESHOLD_PERCENTAGE = 30; |
| * | ||
| * @return the metric name. | ||
| */ | ||
| public String getName() { |
Contributor
There was a problem hiding this comment.
This method and below one should be annotated with @OverRide
| * | ||
| * @return the metric name. | ||
| */ | ||
| public String getName() { |
| abfsUriQueryBuilder, cachedSasToken); | ||
|
|
||
| // Retrieve the read thread pool metrics from the ABFS counters. | ||
| AbfsReadResourceUtilizationMetrics readResourceUtilizationMetrics = retrieveReadResourceUtilizationMetrics(); |
Contributor
There was a problem hiding this comment.
This part of code is common in both DFS and blob client, we can define a method in abfs client class which will add the metrics to tracing Context and that method will be called from both the places.
Contributor
Author
There was a problem hiding this comment.
already defined the common method retrieveReadResourceUtilizationMetrics in abfs client class, tracing context changes we always do in respective client methods only as per the previous API's. Can take this up if still needed
| case TWO_ID_FORMAT: | ||
| header = TracingHeaderVersion.getCurrentVersion() + COLON | ||
| + clientCorrelationID + COLON + clientRequestId; | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : EMPTY_STRING; |
Contributor
There was a problem hiding this comment.
Why have we removed this?
Contributor
Author
There was a problem hiding this comment.
So, the metricResults would be empty string unless anything added to it. As per latest version changes it should either be some string, else emptyString. Hence, I made this change:- metricResults + COLON + resourceUtilizationMetricResults;
|
|
||
| static ReadBufferManagerV2 getBufferManager() { | ||
| /** | ||
| * Returns the singleton instance of {@code ReadBufferManagerV2} for the given ABFS client. |
Contributor
There was a problem hiding this comment.
Comment still need to be fixed
| * @param abfsCounters the {@link AbfsCounters} used for managing read operations. | ||
| */ | ||
| private ReadBufferManagerV2() { | ||
| private ReadBufferManagerV2(AbfsCounters abfsCounters) { |
Contributor
There was a problem hiding this comment.
This constructor will be called only once for all the FSs
Are abfsCounters and readThreadPoolMetrics also singleton?
What if these are different for different Filesystems?
|
|
||
| /** | ||
| * Private constructor to prevent instantiation as this needs to be singleton. | ||
| * Initializes a new instance of {@code ReadBufferManagerV2} for the given ABFS client. |
| poolSizeManager.startCPUMonitoring(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Was that supposed to be taken separately?
| * Schema: version:clientCorrelationId:clientRequestId:fileSystemId | ||
| * :primaryRequestId:streamId:opType:retryHeader:ingressHandler | ||
| * :position:operatedBlobCount:operationSpecificHeader:httpOperationHeader | ||
| * :networkLibrary:operationMetrics |
Contributor
There was a problem hiding this comment.
Earlier it was 13 right?
We are only adding one more in this PR.
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces new performance metrics in the ABFS driver to monitor and evaluate the effectiveness of read and write aggressiveness tuning. These metrics help in understanding how thread pool behavior, CPU utilization, and heap availability impact overall I/O throughput and latency. By capturing detailed statistics such as active thread count, pool size, and system resource utilization, this enhancement enables data-driven analysis of optimizations made to improve ABFS read and write performance under varying workloads.