[GitHub] [hadoop] steveloughran commented on a change in pull request #2353: HADOOP-17272. ABFS Streams to support IOStatistics API

[GitBox <gi...@apache.org>]

steveloughran commented on a change in pull request #2353:
URL: https://github.com/apache/hadoop/pull/2353#discussion_r500464223



##########
File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java
##########
@@ -279,7 +289,15 @@ int readRemote(long position, byte[] b, int offset, int length) throws IOExcepti
     AbfsPerfTracker tracker = client.getAbfsPerfTracker();
     try (AbfsPerfInfo perfInfo = new AbfsPerfInfo(tracker, "readRemote", "read")) {
       LOG.trace("Trigger client.read for path={} position={} offset={} length={}", path, position, offset, length);
-      op = client.read(path, position, b, offset, length, tolerateOobAppends ? "*" : eTag, cachedSasToken.get());
+      if (ioStatistics != null) {
+        op = IOStatisticsBinding.trackDuration((IOStatisticsStore) ioStatistics,

Review comment:
       I'm going to make trackDuration actually apply the operation, have a different method to create a wrapper callable

##########
File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStreamStatisticsImpl.java
##########
@@ -161,55 +170,92 @@ public void remoteBytesRead(long bytes) {
    */
   @Override
   public void remoteReadOperation() {
-    remoteReadOperations++;
+    ioStatisticsStore.incrementCounter(getStatName(REMOTE_READ_OP));
   }
 
+  /**
+   * Getter for IOStatistics instance used.
+   * @return IOStatisticsStore instance which extends IOStatistics.
+   */
+  @Override
+  public IOStatistics getIOStatistics() {
+    return ioStatisticsStore;
+  }
+
+  @VisibleForTesting
   public long getSeekOperations() {
-    return seekOperations;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_SEEK_OPERATIONS);
   }
 
+  @VisibleForTesting
   public long getForwardSeekOperations() {
-    return forwardSeekOperations;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_SEEK_FORWARD_OPERATIONS);
   }
 
+  @VisibleForTesting
   public long getBackwardSeekOperations() {
-    return backwardSeekOperations;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_SEEK_BACKWARD_OPERATIONS);
   }
 
+  @VisibleForTesting
   public long getBytesRead() {
-    return bytesRead;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_BYTES);
   }
 
+  @VisibleForTesting
   public long getBytesSkippedOnSeek() {
-    return bytesSkippedOnSeek;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_SEEK_BYTES_SKIPPED);
   }
 
-  public long getBytesBackwardsOnSeek() {
-    return bytesBackwardsOnSeek;
+  @VisibleForTesting
+  public long getNegativeBytesBackwardsOnSeek() {
+    return ioStatisticsStore.counters().get(getStatName(NEGATIVE_SEEK_BYTES_BACKWARDS));
   }
 
+  @VisibleForTesting
   public long getSeekInBuffer() {
-    return seekInBuffer;
+    return ioStatisticsStore.counters().get(getStatName(SEEK_IN_BUFFER));
+
   }
 
+  @VisibleForTesting
   public long getReadOperations() {
-    return readOperations;
+    return ioStatisticsStore.counters().get(StreamStatisticNames.STREAM_READ_OPERATIONS);

Review comment:
       consider static imports for the statistic names

##########
File path: hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsInputStreamStatistics.java
##########
@@ -160,7 +159,7 @@ public void testSeekStatistics() throws IOException {
       assertEquals("Mismatch in forwardSeekOps value", OPERATIONS,
           stats.getForwardSeekOperations());
       assertEquals("Mismatch in bytesBackwardsOnSeek value",
-          -1 * OPERATIONS * ONE_MB, stats.getBytesBackwardsOnSeek());
+          OPERATIONS * ONE_MB, stats.getNegativeBytesBackwardsOnSeek());

Review comment:
       why the name change? What does this statistic mean?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org