hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mbau...@apache.org
Subject svn commit: r1310973 - in /hbase/branches/0.89-fb/src: main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
Date Sun, 08 Apr 2012 10:38:17 GMT
Author: mbautin
Date: Sun Apr  8 10:38:17 2012
New Revision: 1310973

URL: http://svn.apache.org/viewvc?rev=1310973&view=rev
Log:
[jira] [HBASE-5744] [89-fb] Thrift server metrics should be long instead of int

Summary: As we measure our Thrift call latencies in nanoseconds, we need to make
latencies long instead of int everywhere. There is a bug where we truncate a
nanosecond latency to int, which is a problem with RPCs that take more than
2.147483647 seconds to process.

Test Plan:
TestThriftServer is updated to test for the failure case (an RPC is artificially
made to take 3 seconds). The new test case fails without the fix.

Re-run all unit tests.

Reviewers: schen, dhruba, kannan, liyintang

Reviewed By: schen

CC: stack

Differential Revision: https://reviews.facebook.net/D2679

Modified:
    hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
    hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
    hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java

Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java?rev=1310973&r1=1310972&r2=1310973&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
(original)
+++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java
Sun Apr  8 10:38:17 2012
@@ -63,7 +63,7 @@ public class HbaseHandlerMetricsProxy im
     try {
       long start = now();
       result = m.invoke(handler, args);
-      int processTime = (int)(now() - start);
+      long processTime = now() - start;
       metrics.incMethodTime(m.getName(), processTime);
     } catch (InvocationTargetException e) {
       throw e.getTargetException();

Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java?rev=1310973&r1=1310972&r2=1310973&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
(original)
+++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java
Sun Apr  8 10:38:17 2012
@@ -101,16 +101,16 @@ public class ThriftMetrics implements Up
     numBatchMutateRowKeys.inc(diff);
   }
 
-  public void incMethodTime(String name, int time) {
-    MetricsTimeVaryingRate methodTimeMetrc = getMethodTimeMetrics(name);
-    if (methodTimeMetrc == null) {
+  public void incMethodTime(String name, long time) {
+    MetricsTimeVaryingRate methodTimeMetric = getMethodTimeMetrics(name);
+    if (methodTimeMetric == null) {
       LOG.warn(
           "Got incMethodTime() request for method that doesnt exist: " + name);
       return; // ignore methods that dont exist.
     }
 
     // inc method specific processTime
-    methodTimeMetrc.inc(time);
+    methodTimeMetric.inc(time);
 
     // inc general processTime
     thriftCall.inc(time);

Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java?rev=1310973&r1=1310972&r2=1310973&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
(original)
+++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
Sun Apr  8 10:38:17 2012
@@ -28,10 +28,12 @@ import org.apache.hadoop.hbase.HBaseClus
 import org.apache.hadoop.hbase.thrift.generated.BatchMutation;
 import org.apache.hadoop.hbase.thrift.generated.ColumnDescriptor;
 import org.apache.hadoop.hbase.thrift.generated.Hbase;
+import org.apache.hadoop.hbase.thrift.generated.IOError;
 import org.apache.hadoop.hbase.thrift.generated.Mutation;
 import org.apache.hadoop.hbase.thrift.generated.TCell;
 import org.apache.hadoop.hbase.thrift.generated.TRowResult;
 import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.util.Threads;
 import org.apache.hadoop.metrics.ContextFactory;
 import org.apache.hadoop.metrics.MetricsContext;
 import org.apache.hadoop.metrics.MetricsUtil;
@@ -105,12 +107,27 @@ public class TestThriftServer extends HB
     handler.deleteTable(tableAname);
   }
 
+  public static final class MySlowHBaseHandler extends ThriftServer.HBaseHandler
+      implements Hbase.Iface {
+
+    protected MySlowHBaseHandler(Configuration c, ThriftMetrics metrics)
+        throws IOException {
+      super(c, metrics);
+    }
+
+    @Override
+    public List<byte[]> getTableNames() throws IOError {
+      Threads.sleepWithoutInterrupt(3000);
+      return super.getTableNames();
+    }
+  }
+
   /**
    * Tests if the metrics for thrift handler work correctly
    */
   public void doTestThriftMetrics() throws Exception {
     ThriftMetrics metrics = getMetrics(conf);
-    Hbase.Iface handler = getHandler(metrics, conf);
+    Hbase.Iface handler = getHandlerForMetricsTest(metrics, conf);
     handler.createTable(tableAname, getColumnDescriptors());
     handler.disableTable(tableAname);
     handler.deleteTable(tableAname);
@@ -120,12 +137,16 @@ public class TestThriftServer extends HB
     verifyMetrics(metrics, "createTable_num_ops", 2);
     verifyMetrics(metrics, "deleteTable_num_ops", 2);
     verifyMetrics(metrics, "disableTable_num_ops", 2);
+    handler.getTableNames(); // This will have an artificial delay.
+
+    // 3 to 6 seconds (to account for potential slowness), measured in nanoseconds.
+    verifyMetricRange(metrics, "getTableNames_avg_time", 3L * 1000 * 1000 * 1000,
+        6L * 1000 * 1000 * 1000);
   }
 
-  private static Hbase.Iface getHandler(ThriftMetrics metrics, Configuration conf)
+  private static Hbase.Iface getHandlerForMetricsTest(ThriftMetrics metrics, Configuration
conf)
       throws Exception {
-    Hbase.Iface handler =
-      new ThriftServer.HBaseHandler(conf);
+    Hbase.Iface handler = new MySlowHBaseHandler(conf, metrics);
     return HbaseHandlerMetricsProxy.newInstance(handler, metrics, conf);
   }
 
@@ -143,14 +164,29 @@ public class TestThriftServer extends HB
                .createRecord(ThriftMetrics.CONTEXT_NAME).remove();
   }
 
-  private static void verifyMetrics(ThriftMetrics metrics, String name, int expectValue)
+  private static void verifyMetrics(ThriftMetrics metrics, String name, long expectValue)
       throws Exception {
+    long metricVal = getMetricValue(metrics, name);
+    assertEquals(expectValue, metricVal);
+  }
+
+  private static void verifyMetricRange(ThriftMetrics metrics, String name,
+      long minValue, long maxValue)
+      throws Exception {
+    long metricVal = getMetricValue(metrics, name);
+    if (metricVal < minValue || metricVal > maxValue) {
+      throw new AssertionError("Value of metric " + name + " is outside of the expected "
+
+          "range [" +  minValue + ", " + maxValue + "]: " + metricVal);
+    }
+  }
+
+  private static long getMetricValue(ThriftMetrics metrics, String name) {
     MetricsContext context = MetricsUtil.getContext(
         ThriftMetrics.CONTEXT_NAME);
     metrics.doUpdates(context);
     OutputRecord record = context.getAllRecords().get(
         ThriftMetrics.CONTEXT_NAME).iterator().next();
-    assertEquals(expectValue, record.getMetric(name).intValue());
+    return record.getMetric(name).longValue();
   }
 
   /**



Mime
View raw message