diff --git a/common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java b/common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java index c3949f2..b939945 100644 --- a/common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java +++ b/common/src/java/org/apache/hadoop/hive/common/JvmPauseMonitor.java @@ -199,10 +199,12 @@ public void run() { } private void incrementMetricsCounter(String name, long count) { - try { - MetricsFactory.getMetricsInstance().incrementCounter(name, count); - } catch (Exception e) { - LOG.warn("Error Reporting JvmPauseMonitor to Metrics system", e); + if (MetricsFactory.getInstance() != null) { + try { + MetricsFactory.getInstance().incrementCounter(name, count); + } catch (Exception e) { + LOG.warn("Error Reporting JvmPauseMonitor to Metrics system", e); + } } } } diff --git a/common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java b/common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java index 14f7afb..e811339 100644 --- a/common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java +++ b/common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java @@ -149,7 +149,6 @@ public void reopen() throws IOException { } } - private static final ThreadLocal> threadLocalScopes = new ThreadLocal>() { @Override @@ -158,31 +157,16 @@ public void reopen() throws IOException { } }; - private boolean initialized = false; - - public void init(HiveConf conf) throws Exception { - if (!initialized) { - MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); - mbs.registerMBean(metrics, oname); - initialized = true; - } - } - - public boolean isInitialized() { - return initialized; + public LegacyMetrics(HiveConf conf) throws Exception { + MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); + mbs.registerMBean(metrics, oname); } public Long incrementCounter(String name) throws IOException{ - if (!initialized) { - return null; - } return incrementCounter(name,Long.valueOf(1)); } public Long incrementCounter(String name, long increment) throws IOException{ - if (!initialized) { - return null; - } Long value; synchronized(metrics) { if (!metrics.hasKey(name)) { @@ -197,23 +181,14 @@ public Long incrementCounter(String name, long increment) throws IOException{ } public void set(String name, Object value) throws IOException{ - if (!initialized) { - return; - } metrics.put(name,value); } public Object get(String name) throws IOException{ - if (!initialized) { - return null; - } return metrics.get(name); } public void startScope(String name) throws IOException{ - if (!initialized) { - return; - } if (threadLocalScopes.get().containsKey(name)) { threadLocalScopes.get().get(name).open(); } else { @@ -222,9 +197,6 @@ public void startScope(String name) throws IOException{ } public MetricsScope getScope(String name) throws IOException { - if (!initialized) { - return null; - } if (threadLocalScopes.get().containsKey(name)) { return threadLocalScopes.get().get(name); } else { @@ -233,9 +205,6 @@ public MetricsScope getScope(String name) throws IOException { } public void endScope(String name) throws IOException{ - if (!initialized) { - return; - } if (threadLocalScopes.get().containsKey(name)) { threadLocalScopes.get().get(name).close(); } @@ -247,16 +216,14 @@ public void endScope(String name) throws IOException{ * * Note that threadLocalScopes ThreadLocal is *not* cleared in this call. */ - public void deInit() throws Exception { + public void close() throws Exception { synchronized (metrics) { - if (initialized) { - MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); - if (mbs.isRegistered(oname)) { - mbs.unregisterMBean(oname); - } - metrics.clear(); - initialized = false; + MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); + if (mbs.isRegistered(oname)) { + mbs.unregisterMBean(oname); } + metrics.clear(); + threadLocalScopes.remove(); } } } diff --git a/common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java b/common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java index 13a5336..27b69cc 100644 --- a/common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java +++ b/common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java @@ -28,16 +28,12 @@ */ public interface Metrics { - /** - * Initialize Metrics system with given Hive configuration. - * @param conf - */ - public void init(HiveConf conf) throws Exception; + //Must declare CTOR taking in HiveConf. /** * Deinitializes the Metrics system. */ - public void deInit() throws Exception; + public void close() throws Exception; /** * @param name diff --git a/common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsFactory.java b/common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsFactory.java index 12a309d..8769d68 100644 --- a/common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsFactory.java +++ b/common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsFactory.java @@ -20,29 +20,43 @@ import org.apache.hadoop.hive.conf.HiveConf; import org.apache.hadoop.util.ReflectionUtils; +import java.lang.reflect.Constructor; + /** * Class that manages a static Metric instance for this process. */ public class MetricsFactory { - private static Metrics metrics; - private static Object initLock = new Object(); + //Volatile ensures that static access returns Metrics instance in fully-initialized state. + //Alternative is to synchronize static access, which has performance penalties. + private volatile static Metrics metrics; + /** + * Initializes static Metrics instance. + */ public synchronized static void init(HiveConf conf) throws Exception { if (metrics == null) { - metrics = (Metrics) ReflectionUtils.newInstance(conf.getClassByName( - conf.getVar(HiveConf.ConfVars.HIVE_METRICS_CLASS)), conf); + Class metricsClass = conf.getClassByName( + conf.getVar(HiveConf.ConfVars.HIVE_METRICS_CLASS)); + Constructor constructor = metricsClass.getConstructor(HiveConf.class); + metrics = (Metrics) constructor.newInstance(conf); } - metrics.init(conf); } - public synchronized static Metrics getMetricsInstance() { + /** + * Returns static Metrics instance, null if not initialized or closed. + */ + public static Metrics getInstance() { return metrics; } - public synchronized static void deInit() throws Exception { + /** + * Closes and removes static Metrics instance. + */ + public synchronized static void close() throws Exception { if (metrics != null) { - metrics.deInit(); + metrics.close(); + metrics = null; } } } diff --git a/common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java b/common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java index e59da99..ae353d0 100644 --- a/common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java +++ b/common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java @@ -77,7 +77,6 @@ private LoadingCache timers; private LoadingCache counters; - private boolean initialized = false; private HiveConf conf; private final Set reporters = new HashSet(); @@ -139,11 +138,7 @@ public void close() throws IOException { } } - public synchronized void init(HiveConf conf) throws Exception { - if (initialized) { - return; - } - + public CodahaleMetrics(HiveConf conf) throws Exception { this.conf = conf; //Codahale artifacts are lazily-created. timers = CacheBuilder.newBuilder().build( @@ -190,32 +185,23 @@ public Counter load(String key) throws Exception { } } initReporting(finalReporterList); - initialized = true; } - public synchronized void deInit() throws Exception { - if (initialized) { - if (reporters != null) { - for (Closeable reporter : reporters) { - reporter.close(); - } + public void close() throws Exception { + if (reporters != null) { + for (Closeable reporter : reporters) { + reporter.close(); } - for (Map.Entry metric : metricRegistry.getMetrics().entrySet()) { - metricRegistry.remove(metric.getKey()); - } - timers.invalidateAll(); - counters.invalidateAll(); - initialized = false; } + for (Map.Entry metric : metricRegistry.getMetrics().entrySet()) { + metricRegistry.remove(metric.getKey()); + } + timers.invalidateAll(); + counters.invalidateAll(); } public void startScope(String name) throws IOException { - synchronized (this) { - if (!initialized) { - return; - } - } name = API_PREFIX + name; if (threadLocalScopes.get().containsKey(name)) { threadLocalScopes.get().get(name).open(); @@ -224,12 +210,7 @@ public void startScope(String name) throws IOException { } } - public void endScope(String name) throws IOException{ - synchronized (this) { - if (!initialized) { - return; - } - } + public void endScope(String name) throws IOException { name = API_PREFIX + name; if (threadLocalScopes.get().containsKey(name)) { threadLocalScopes.get().get(name).close(); diff --git a/common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java b/common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java index c14c7ee..c3e8282 100644 --- a/common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java +++ b/common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java @@ -47,16 +47,16 @@ @Before public void before() throws Exception { - MetricsFactory.deInit(); + MetricsFactory.close(); HiveConf conf = new HiveConf(); conf.setVar(HiveConf.ConfVars.HIVE_METRICS_CLASS, LegacyMetrics.class.getCanonicalName()); MetricsFactory.init(conf); - metrics = (LegacyMetrics) MetricsFactory.getMetricsInstance(); + metrics = (LegacyMetrics) MetricsFactory.getInstance(); } @After public void after() throws Exception { - MetricsFactory.deInit(); + MetricsFactory.close(); } @Test diff --git a/common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java b/common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java index 8749349..954b388 100644 --- a/common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java +++ b/common/src/test/org/apache/hadoop/hive/common/metrics/metrics2/TestCodahaleMetrics.java @@ -63,20 +63,20 @@ public void before() throws Exception { conf.setVar(HiveConf.ConfVars.HIVE_METRICS_JSON_FILE_INTERVAL, "100ms"); MetricsFactory.init(conf); - metricRegistry = ((CodahaleMetrics) MetricsFactory.getMetricsInstance()).getMetricRegistry(); + metricRegistry = ((CodahaleMetrics) MetricsFactory.getInstance()).getMetricRegistry(); } @After public void after() throws Exception { - MetricsFactory.deInit(); + MetricsFactory.close(); } @Test public void testScope() throws Exception { int runs = 5; for (int i = 0; i < runs; i++) { - MetricsFactory.getMetricsInstance().startScope("method1"); - MetricsFactory.getMetricsInstance().endScope("method1"); + MetricsFactory.getInstance().startScope("method1"); + MetricsFactory.getInstance().endScope("method1"); } Timer timer = metricRegistry.getTimers().get("api_method1"); @@ -89,7 +89,7 @@ public void testScope() throws Exception { public void testCount() throws Exception { int runs = 5; for (int i = 0; i < runs; i++) { - MetricsFactory.getMetricsInstance().incrementCounter("count1"); + MetricsFactory.getInstance().incrementCounter("count1"); } Counter counter = metricRegistry.getCounters().get("count1"); Assert.assertEquals(5L, counter.getCount()); @@ -104,8 +104,8 @@ public void testConcurrency() throws Exception { executorService.submit(new Callable() { @Override public Void call() throws Exception { - MetricsFactory.getMetricsInstance().startScope("method2"); - MetricsFactory.getMetricsInstance().endScope("method2"); + MetricsFactory.getInstance().startScope("method2"); + MetricsFactory.getInstance().endScope("method2"); return null; } }); @@ -121,7 +121,7 @@ public Void call() throws Exception { public void testFileReporting() throws Exception { int runs = 5; for (int i = 0; i < runs; i++) { - MetricsFactory.getMetricsInstance().incrementCounter("count2"); + MetricsFactory.getInstance().incrementCounter("count2"); Thread.sleep(100); } diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java index 85a734c..837e925 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java @@ -748,9 +748,9 @@ private String startFunction(String function, String extraLogInfo) { incrementCounter(function); logInfo((getIpAddress() == null ? "" : "source:" + getIpAddress() + " ") + function + extraLogInfo); - if (hiveConf.getBoolVar(ConfVars.METASTORE_METRICS)) { + if (MetricsFactory.getInstance() != null) { try { - MetricsFactory.getMetricsInstance().startScope(function); + MetricsFactory.getInstance().startScope(function); } catch (IOException e) { LOG.debug("Exception when starting metrics scope" + e.getClass().getName() + " " + e.getMessage(), e); @@ -792,9 +792,9 @@ private void endFunction(String function, boolean successful, Exception e, } private void endFunction(String function, MetaStoreEndFunctionContext context) { - if (hiveConf.getBoolVar(ConfVars.METASTORE_METRICS)) { + if (MetricsFactory.getInstance() != null) { try { - MetricsFactory.getMetricsInstance().endScope(function); + MetricsFactory.getInstance().endScope(function); } catch (IOException e) { LOG.debug("Exception when closing metrics scope" + e); } @@ -823,7 +823,7 @@ public void shutdown() { } if (hiveConf.getBoolVar(ConfVars.METASTORE_METRICS)) { try { - MetricsFactory.deInit(); + MetricsFactory.close(); } catch (Exception e) { LOG.error("error in Metrics deinit: " + e.getClass().getName() + " " + e.getMessage(), e); diff --git a/service/src/java/org/apache/hive/service/server/HiveServer2.java b/service/src/java/org/apache/hive/service/server/HiveServer2.java index 7820ed5..14cab68 100644 --- a/service/src/java/org/apache/hive/service/server/HiveServer2.java +++ b/service/src/java/org/apache/hive/service/server/HiveServer2.java @@ -307,9 +307,9 @@ public synchronized void stop() { HiveConf hiveConf = this.getHiveConf(); super.stop(); // Shutdown Metrics - if (hiveConf.getBoolVar(ConfVars.HIVE_SERVER2_METRICS_ENABLED)) { + if (MetricsFactory.getInstance() != null) { try { - MetricsFactory.getMetricsInstance().deInit(); + MetricsFactory.close(); } catch (Exception e) { LOG.error("error in Metrics deinit: " + e.getClass().getName() + " " + e.getMessage(), e); @@ -355,8 +355,8 @@ private static void startHiveServer2() throws Throwable { server.init(hiveConf); server.start(); - if (hiveConf.getBoolVar(ConfVars.HIVE_SERVER2_METRICS_ENABLED)) { - MetricsFactory.getMetricsInstance().init(hiveConf); + if (hiveConf.getBoolVar(ConfVars.HIVE_SERVER2_METRICS_ENABLED) && MetricsFactory.getInstance() == null) { + MetricsFactory.init(hiveConf); } try { JvmPauseMonitor pauseMonitor = new JvmPauseMonitor(hiveConf);