[GitHub] [hbase] clarax commented on a change in pull request #1781: HBASE-24435 Add hedgedReads and hedgedReadWins count metrics

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

clarax commented on a change in pull request #1781:
URL: https://github.com/apache/hbase/pull/1781#discussion_r430772485



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
##########
@@ -592,9 +593,9 @@ public void markRegionsRecovering(ServerName server, Set<HRegionInfo> userRegion
    * @return whether log is replaying
    */
   public boolean isLogReplaying() {
-    if (server.getCoordinatedStateManager() == null) return false;
-    return ((BaseCoordinatedStateManager) server.getCoordinatedStateManager())
-        .getSplitLogManagerCoordination().isReplaying();
+    CoordinatedStateManager m = server.getCoordinatedStateManager();
+    if (m == null) return false;

Review comment:
       Checkstyle conflicts need to be fixed by adding braces.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
##########
@@ -1780,4 +1782,47 @@ public static void checkShortCircuitReadBufferSize(final Configuration conf) {
     int hbaseSize = conf.getInt("hbase." + dfsKey, defaultSize);
     conf.setIfUnset(dfsKey, Integer.toString(hbaseSize));
   }
+
+  /**
+   * @param c
+   * @return The DFSClient DFSHedgedReadMetrics instance or null if can't be found or not on hdfs.
+   * @throws IOException

Review comment:
       can be removed

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
##########
@@ -1780,4 +1782,47 @@ public static void checkShortCircuitReadBufferSize(final Configuration conf) {
     int hbaseSize = conf.getInt("hbase." + dfsKey, defaultSize);
     conf.setIfUnset(dfsKey, Integer.toString(hbaseSize));
   }
+
+  /**
+   * @param c

Review comment:
       can be removed.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
##########
@@ -1780,4 +1782,47 @@ public static void checkShortCircuitReadBufferSize(final Configuration conf) {
     int hbaseSize = conf.getInt("hbase." + dfsKey, defaultSize);
     conf.setIfUnset(dfsKey, Integer.toString(hbaseSize));
   }
+
+  /**
+   * @param c
+   * @return The DFSClient DFSHedgedReadMetrics instance or null if can't be found or not on hdfs.
+   * @throws IOException
+   */
+  public static DFSHedgedReadMetrics getDFSHedgedReadMetrics(final Configuration c)
+      throws IOException {
+    if (!isHDFS(c)) return null;

Review comment:
       Checkstyle conflict. Need braces.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperImpl.java
##########
@@ -40,7 +41,9 @@
 import org.apache.hadoop.hbase.io.hfile.CacheStats;
 import org.apache.hadoop.hbase.wal.WALProvider;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.hadoop.hbase.util.FSUtils;
 import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher;
+import org.apache.hadoop.hdfs.DFSHedgedReadMetrics;

Review comment:
       This is from 2.4. https://issues.apache.org/jira/browse/HBASE-7509 Should be good.
   

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
##########
@@ -435,4 +440,146 @@ public void checkStreamCapabilitiesOnHdfsDataOutputStream() throws Exception {
     }
   }
 
+  /**
+   * Ugly test that ensures we can get at the hedged read counters in dfsclient.
+   * Does a bit of preading with hedged reads enabled using code taken from hdfs TestPread.
+   * @throws Exception

Review comment:
       Not necessary.

##########
File path: hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceImpl.java
##########
@@ -504,6 +504,11 @@ public void getMetrics(MetricsCollector metricsCollector, boolean all) {
               rsWrap.getCompactedCellsSize())
           .addCounter(Interns.info(MAJOR_COMPACTED_CELLS_SIZE, MAJOR_COMPACTED_CELLS_SIZE_DESC),
               rsWrap.getMajorCompactedCellsSize())
+
+          .addCounter(Interns.info(HEDGED_READS, HEDGED_READS_DESC), rsWrap.getHedgedReadOps())
+          .addCounter(Interns.info(HEDGED_READ_WINS, HEDGED_READ_WINS_DESC),
+              rsWrap.getHedgedReadWins())

Review comment:
       Validated from HBASE15550. LGTM.




----------------------------------------------------------------
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