[
https://issues.apache.org/jira/browse/HADOOP-19737?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18039830#comment-18039830
]
ASF GitHub Bot commented on HADOOP-19737:
-----------------------------------------
anujmodi2021 commented on code in PR #8056:
URL: https://github.com/apache/hadoop/pull/8056#discussion_r2549241207
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/enums/AbfsWriteThreadPoolMetricsEnum.java:
##########
@@ -0,0 +1,95 @@
+/**
Review Comment:
Do we need 2 searate classes for these?
All the metrics seems to be common in both.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV2.java:
##########
@@ -104,14 +109,39 @@ public final class ReadBufferManagerV2 extends
ReadBufferManager {
private static AtomicBoolean isConfigured = new AtomicBoolean(false);
+ /* Metrics collector for monitoring the performance of the ABFS read thread
pool. */
+ private final AbfsReadThreadPoolMetrics readThreadPoolMetrics;
+
+ /* Last recorded CPU time used for computing CPU utilization deltas. */
+ private static long lastCpuTime = 0;
+
+ /* Last recorded system time used for utilization calculations. */
+ private static long lastTime = 0;
+
+ private final AbfsClient abfsClient;
+ /* Tracks the last scale direction applied, or empty if none. */
+ private volatile String lastScaleDirection = EMPTY_STRING;
+ /* Maximum CPU utilization observed during the monitoring interval. */
+ private volatile double maxCpuUtilization = 0.0;
+
/**
- * Private constructor to prevent instantiation as this needs to be
singleton.
+ * Initializes a new instance of {@code ReadBufferManagerV2} for the given
ABFS client.
+ *
+ * @param abfsClient the {@link AbfsClient} used for managing read
operations.
*/
- private ReadBufferManagerV2() {
+ private ReadBufferManagerV2(AbfsClient abfsClient) {
+ this.abfsClient = abfsClient;
Review Comment:
Why do we need client here?
IMO we should not pass client to RBM.
Any client related work should happen in AbfsInputStream how its happening
today.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -1346,7 +1346,13 @@ public AbfsRestOperation read(final String path,
final AbfsUriQueryBuilder abfsUriQueryBuilder =
createDefaultUriQueryBuilder();
String sasTokenForReuse = appendSASTokenToQuery(path,
SASTokenProvider.READ_OPERATION,
abfsUriQueryBuilder, cachedSasToken);
-
+ // Retrieve the read thread pool metrics from the ABFS counters.
+ AbfsReadThreadPoolMetrics metrics = getAbfsCounters()
Review Comment:
Common code in Blob and Dfs client Can be a common method in base class.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV2.java:
##########
@@ -104,14 +109,39 @@ public final class ReadBufferManagerV2 extends
ReadBufferManager {
private static AtomicBoolean isConfigured = new AtomicBoolean(false);
+ /* Metrics collector for monitoring the performance of the ABFS read thread
pool. */
+ private final AbfsReadThreadPoolMetrics readThreadPoolMetrics;
+
+ /* Last recorded CPU time used for computing CPU utilization deltas. */
+ private static long lastCpuTime = 0;
+
+ /* Last recorded system time used for utilization calculations. */
+ private static long lastTime = 0;
+
+ private final AbfsClient abfsClient;
+ /* Tracks the last scale direction applied, or empty if none. */
+ private volatile String lastScaleDirection = EMPTY_STRING;
+ /* Maximum CPU utilization observed during the monitoring interval. */
+ private volatile double maxCpuUtilization = 0.0;
+
/**
- * Private constructor to prevent instantiation as this needs to be
singleton.
+ * Initializes a new instance of {@code ReadBufferManagerV2} for the given
ABFS client.
Review Comment:
All the clients in JVM ae supposed to use same ReadBufferManager. This
comment seems wrong
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AzureDFSIngressHandler.java:
##########
@@ -117,6 +117,13 @@ protected AbfsRestOperation remoteWrite(AbfsBlock
blockToUpload,
AppendRequestParameters reqParams,
TracingContext tracingContext) throws IOException {
TracingContext tracingContextAppend = new TracingContext(tracingContext);
+ // Fetches write thread pool metrics from the ABFS client and adds them to
the tracing context.
Review Comment:
Common code Can be moved to base IngressHandler
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java:
##########
@@ -242,6 +253,18 @@ public AzureIngressHandler getIngressHandler() {
private volatile boolean switchCompleted = false;
+ /**
+ * Starts CPU monitoring in the thread pool size manager if it
+ * is initialized and not already monitoring.
+ */
+ private void initializeMonitoringIfNeeded() {
+ if (poolSizeManager != null && !poolSizeManager.isMonitoringStarted()) {
Review Comment:
Do we need to repeat this check inside synchronized block as well?
What if 2 threads check together and both try to startCPUMonitoring?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -1346,7 +1346,13 @@ public AbfsRestOperation read(final String path,
final AbfsUriQueryBuilder abfsUriQueryBuilder =
createDefaultUriQueryBuilder();
String sasTokenForReuse = appendSASTokenToQuery(path,
SASTokenProvider.READ_OPERATION,
abfsUriQueryBuilder, cachedSasToken);
-
+ // Retrieve the read thread pool metrics from the ABFS counters.
Review Comment:
Don't we need a similar change in append as well?
##########
hadoop-tools/hadoop-azure/pom.xml:
##########
@@ -632,6 +632,8 @@
<exclude>**/azurebfs/ITestSmallWriteOptimization.java</exclude>
<exclude>**/azurebfs/ITestAbfsStreamStatistics*.java</exclude>
<exclude>**/azurebfs/services/ITestReadBufferManager.java</exclude>
+
<exclude>**/azurebfs/WriteThreadPoolSizeManager.java</exclude>
Review Comment:
Why this change?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsWriteThreadPoolMetrics.java:
##########
@@ -0,0 +1,165 @@
+/**
Review Comment:
Same here.
A lot ff redundant code can be combined.
> ABFS: Add metrics to identify improvements with read and write aggressiveness
> -----------------------------------------------------------------------------
>
> Key: HADOOP-19737
> URL: https://issues.apache.org/jira/browse/HADOOP-19737
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.5.0, 3.4.2
> Reporter: Anmol Asrani
> Assignee: Anmol Asrani
> Priority: Major
> Labels: pull-request-available
>
> 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
