Thanks for contributing this improvement Hanisha Koneru. A few comments:

1. For maintaining stats one more option is RollingAverages which uses MutableRatesWithAggregation and is optimized for multithreaded updates.
2. Instead of floating point arithmetic here:

   sampleRangeMax = (int) ((double) conf.getScrMetricsSamplingPercentage()
           / 100 * Integer.MAX_VALUE);
   

   Alternatively:

   sampleRangeMax = (Integer.MAX_VALUE / 100) * conf.getScrMetricsSamplingPercentage();
   

   Also we should limit getScrMetricsSamplingPercentage to [0, 100] if the administrator misconfigures it.

3. We should add isolated test cases for BlockReaderIoProvider and BlockReaderLocalMetrics if possible.

Thanks for the review Arpit Agarwal. Patch v02 addresses your comments.

-1 overall

This message was automatically generated.

Thanks for updating the patch Hanisha Koneru. A few comments:

1. Typos in SHORT_CIRCUIT_READ_LATECNY_METRIC_REGISTERD_NAME.
2. The test-only method getShortCircuitReadRollingAverages should be tagged with @VisibleForTesting. Also it should be package-private if possible.
3. I think we can eliminate the METRICS_ENABLED config key. We can enable the metric when the sampling percentage is > 0.
4. Let's avoid static initialization of BlockReaderLocal#metrics. One issue is that adding the overhead of a MutableRollingAverages object (and its rates roller thread) for all clients, most of which may will never enable SCR statistics. I think your original approach of initializing it within a lock on construction was fine, the overhead of that lock compared to the overhead of cloning a file descriptor via system calls should be minimal.

Still reviewing the tests.

Thanks Arpit Agarwal for the review. Posted patch v03 with the updates.

-1 overall

This message was automatically generated.

Thanks for updating the patch Hanisha Koneru. A few more comments:

1. In BlockReaderLocal constructor, metrics is being passed to BlockReaderIoProvider before it is potentially initialized.

       this.blockReaderIoProvider = new BlockReaderIoProvider(
           builder.shortCircuitConf, metrics, timer);
   
       if (builder.shortCircuitConf.isScrMetricsEnabled()) {
         metricsInitializationLock.lock();
         if (!isMetricsEnabled) {
           metrics = BlockReaderLocalMetrics.create();
           isMetricsEnabled = true;
         }
   

2. Typo - SHORT_CIRCUIT_READ_LATECNY_METRIC_REGISTERD_NAME.
3. Looks like we can eliminate the BlockReaderLocal#isMetricsEnabled, instead just check whether metrics is null.
4. Thanks for adding TestBlockReaderIoProvider. One more related test - ensure addShortCircuitReadLatency is not invoked when the read takes less than the threshold.
5. Also I think we can make BlockReaderIoProvider be a static utility class since it doesn't maintain any mutable state. But this approach is also fine since the allocated object is lightweight.

TestBlockReaderLocalMetrics is a nicely written unit test!

Edit: Sorry about the confusion, there is no threshold for addShortCircuitReadLatency.

Thank you Arpit Agarwal.
Patch v04 addresses your comments, checkstyle and findbug errors.

Thanks Hanisha Koneru. +1 pending Jenkins.

-1 overall

This message was automatically generated.

Thanks for the patch revisions Hanisha Koneru!

Can you please add a package-info.java file to address the remaining checkstyle issue? You can take a look at the following package-info files as examples:

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/startupprogress/package-info.java
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/diskbalancer/planner/package-info.java

Thanks for the review Arpit Agarwal. I added the package-info in patch v05.

+1 for the v05 patch, pending Jenkins.

-1 overall

This message was automatically generated.

All the failed junit tests pass locally.

I've committed this. Thank you for the contribution Hanisha Koneru!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11909 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11909/)
HDFS-11789. Maintain Short-Circuit Read Statistics. Contributed by (arp: rev 6d116ffad23b470f8e9ca131d8e89cbbbb4378d7)

• (add) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/client/impl/TestBlockReaderIoProvider.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/impl/DfsClientConf.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/impl/BlockReaderLocal.java
• (add) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/client/impl/TestBlockReaderLocalMetrics.java
• (add) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/impl/metrics/BlockReaderLocalMetrics.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsClientConfigKeys.java
• (add) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/impl/metrics/package-info.java
• (add) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/impl/metrics/BlockReaderIoProvider.java