From f19ce5fcfd9cd479ef5af69977dfd728a0aaecdf Mon Sep 17 00:00:00 2001 From: mbautin Date: Sat, 7 Apr 2012 19:40:40 -0700 Subject: [PATCH] [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 git-svn-id: svn+ssh://tubbs/svnhive/hadoop/branches/titan/VENDOR.hbase/hbase-trunk@24158 e7acf4d4-3532-417f-9e73-7a9ae25a1f51 Conflicts: src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java --- .../hbase/thrift/HbaseHandlerMetricsProxy.java | 2 +- .../apache/hadoop/hbase/thrift/ThriftMetrics.java | 8 ++-- .../hadoop/hbase/thrift/TestThriftServer.java | 54 ++++++++++++++++++-- 3 files changed, 55 insertions(+), 9 deletions(-) diff --git src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java index 039def7..dc43530 100644 --- src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java +++ src/main/java/org/apache/hadoop/hbase/thrift/HbaseHandlerMetricsProxy.java @@ -65,7 +65,7 @@ public class HbaseHandlerMetricsProxy implements InvocationHandler { 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(); diff --git src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java index d6eb5cd..62b0f5a 100644 --- src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java +++ src/main/java/org/apache/hadoop/hbase/thrift/ThriftMetrics.java @@ -98,16 +98,16 @@ public class ThriftMetrics implements Updater { numRowKeysInBatchMutate.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); diff --git src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java index 9e8b3b2..c9cbdfd 100644 --- src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java +++ src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java @@ -40,11 +40,13 @@ import org.apache.hadoop.hbase.filter.ParseFilter; 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.TRegionInfo; 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; @@ -130,24 +132,43 @@ public class TestThriftServer { dropTestTables(handler); } + public static final class MySlowHBaseHandler extends ThriftServerRunner.HBaseHandler + implements Hbase.Iface { + + protected MySlowHBaseHandler(Configuration c) + throws IOException { + super(c); + } + + @Override + public List getTableNames() throws IOError { + Threads.sleepWithoutInterrupt(3000); + return super.getTableNames(); + } + } + /** * Tests if the metrics for thrift handler work correctly */ public void doTestThriftMetrics() throws Exception { Configuration conf = UTIL.getConfiguration(); ThriftMetrics metrics = getMetrics(conf); - Hbase.Iface handler = getHandler(metrics, conf); + Hbase.Iface handler = getHandlerForMetricsTest(metrics, conf); createTestTables(handler); dropTestTables(handler); 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 ThriftServerRunner.HBaseHandler(conf); + Hbase.Iface handler = new MySlowHBaseHandler(conf); return HbaseHandlerMetricsProxy.newInstance(handler, metrics, conf); } @@ -201,6 +222,31 @@ public class TestThriftServer { assertEquals(handler.getTableNames().size(), 0); } + 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(); + return record.getMetric(name).longValue(); + } + /** * Tests adding a series of Mutations and BatchMutations, including a * delete mutation. Also tests data retrieval, and getting back multiple -- 1.7.4.4