From 69d4809d42d2ed86e0c98b9f6b59f13f1b6904c5 Mon Sep 17 00:00:00 2001 From: Elliott Clark Date: Wed, 24 Apr 2013 16:44:11 -0700 Subject: [PATCH] Add more locks to try and keep races from creating 2 metrics. --- .../hbase/CompatibilitySingletonFactory.java | 63 +++++++++++--------- .../MetricsRegionServerSourceFactoryImpl.java | 22 ++++--- .../MetricsRegionServerSourceFactoryImpl.java | 22 ++++--- 3 files changed, 63 insertions(+), 44 deletions(-) diff --git hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/CompatibilitySingletonFactory.java hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/CompatibilitySingletonFactory.java index 2b2c53d..c3f8970 100644 --- hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/CompatibilitySingletonFactory.java +++ hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/CompatibilitySingletonFactory.java @@ -31,8 +31,12 @@ import java.util.ServiceLoader; * created. */ public class CompatibilitySingletonFactory extends CompatibilityFactory { + public static enum SingletonStorage { + INSTANCE; + Object lock = new Object(); + private static final Map instances = new HashMap(); + } private static final Log LOG = LogFactory.getLog(CompatibilitySingletonFactory.class); - private static final Map instances = new HashMap(); /** * This is a static only class don't let anyone create an instance. @@ -45,37 +49,40 @@ public class CompatibilitySingletonFactory extends CompatibilityFactory { * @return the singleton */ @SuppressWarnings("unchecked") - public static synchronized T getInstance(Class klass) { - T instance = (T) instances.get(klass); - if (instance == null) { - try { - ServiceLoader loader = ServiceLoader.load(klass); - Iterator it = loader.iterator(); - instance = it.next(); - if (it.hasNext()) { - StringBuilder msg = new StringBuilder(); - msg.append("ServiceLoader provided more than one implementation for class: ") - .append(klass) - .append(", using implementation: ").append(instance.getClass()) - .append(", other implementations: {"); - while (it.hasNext()) { - msg.append(it.next()).append(" "); + public static T getInstance(Class klass) { + synchronized (SingletonStorage.INSTANCE.lock) { + T instance = (T) SingletonStorage.INSTANCE.instances.get(klass); + if (instance == null) { + try { + ServiceLoader loader = ServiceLoader.load(klass); + Iterator it = loader.iterator(); + instance = it.next(); + if (it.hasNext()) { + StringBuilder msg = new StringBuilder(); + msg.append("ServiceLoader provided more than one implementation for class: ") + .append(klass) + .append(", using implementation: ").append(instance.getClass()) + .append(", other implementations: {"); + while (it.hasNext()) { + msg.append(it.next()).append(" "); + } + msg.append("}"); + LOG.warn(msg); } - msg.append("}"); - LOG.warn(msg); + } catch (Exception e) { + throw new RuntimeException(createExceptionString(klass), e); + } catch (Error e) { + throw new RuntimeException(createExceptionString(klass), e); } - } catch (Exception e) { - throw new RuntimeException(createExceptionString(klass), e); - } catch (Error e) { - throw new RuntimeException(createExceptionString(klass), e); - } - // If there was nothing returned and no exception then throw an exception. - if (instance == null) { - throw new RuntimeException(createExceptionString(klass)); + // If there was nothing returned and no exception then throw an exception. + if (instance == null) { + throw new RuntimeException(createExceptionString(klass)); + } + SingletonStorage.INSTANCE.instances.put(klass, instance); } - instances.put(klass, instance); + return instance; } - return instance; + } } diff --git hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java index dc4ae6a..f6cb79b 100644 --- hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java +++ hbase-hadoop1-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java @@ -22,27 +22,33 @@ package org.apache.hadoop.hbase.regionserver; * Factory to create MetricsRegionServerSource when given a MetricsRegionServerWrapper */ public class MetricsRegionServerSourceFactoryImpl implements MetricsRegionServerSourceFactory { - private static enum FactoryStorage { + public static enum FactoryStorage { INSTANCE; + private Object aggLock = new Object(); + private Object serverLock = new Object(); private MetricsRegionServerSource serverSource; private MetricsRegionAggregateSourceImpl aggImpl; } private synchronized MetricsRegionAggregateSourceImpl getAggregate() { - if (FactoryStorage.INSTANCE.aggImpl == null) { - FactoryStorage.INSTANCE.aggImpl = new MetricsRegionAggregateSourceImpl(); + synchronized (FactoryStorage.INSTANCE.aggLock) { + if (FactoryStorage.INSTANCE.aggImpl == null) { + FactoryStorage.INSTANCE.aggImpl = new MetricsRegionAggregateSourceImpl(); + } + return FactoryStorage.INSTANCE.aggImpl; } - return FactoryStorage.INSTANCE.aggImpl; } @Override public synchronized MetricsRegionServerSource createServer(MetricsRegionServerWrapper regionServerWrapper) { - if (FactoryStorage.INSTANCE.serverSource == null) { - FactoryStorage.INSTANCE.serverSource = new MetricsRegionServerSourceImpl( - regionServerWrapper); + synchronized (FactoryStorage.INSTANCE.serverLock) { + if (FactoryStorage.INSTANCE.serverSource == null) { + FactoryStorage.INSTANCE.serverSource = new MetricsRegionServerSourceImpl( + regionServerWrapper); + } + return FactoryStorage.INSTANCE.serverSource; } - return FactoryStorage.INSTANCE.serverSource; } @Override diff --git hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java index dc4ae6a..f6cb79b 100644 --- hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java +++ hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java @@ -22,27 +22,33 @@ package org.apache.hadoop.hbase.regionserver; * Factory to create MetricsRegionServerSource when given a MetricsRegionServerWrapper */ public class MetricsRegionServerSourceFactoryImpl implements MetricsRegionServerSourceFactory { - private static enum FactoryStorage { + public static enum FactoryStorage { INSTANCE; + private Object aggLock = new Object(); + private Object serverLock = new Object(); private MetricsRegionServerSource serverSource; private MetricsRegionAggregateSourceImpl aggImpl; } private synchronized MetricsRegionAggregateSourceImpl getAggregate() { - if (FactoryStorage.INSTANCE.aggImpl == null) { - FactoryStorage.INSTANCE.aggImpl = new MetricsRegionAggregateSourceImpl(); + synchronized (FactoryStorage.INSTANCE.aggLock) { + if (FactoryStorage.INSTANCE.aggImpl == null) { + FactoryStorage.INSTANCE.aggImpl = new MetricsRegionAggregateSourceImpl(); + } + return FactoryStorage.INSTANCE.aggImpl; } - return FactoryStorage.INSTANCE.aggImpl; } @Override public synchronized MetricsRegionServerSource createServer(MetricsRegionServerWrapper regionServerWrapper) { - if (FactoryStorage.INSTANCE.serverSource == null) { - FactoryStorage.INSTANCE.serverSource = new MetricsRegionServerSourceImpl( - regionServerWrapper); + synchronized (FactoryStorage.INSTANCE.serverLock) { + if (FactoryStorage.INSTANCE.serverSource == null) { + FactoryStorage.INSTANCE.serverSource = new MetricsRegionServerSourceImpl( + regionServerWrapper); + } + return FactoryStorage.INSTANCE.serverSource; } - return FactoryStorage.INSTANCE.serverSource; } @Override -- 1.7.10.2 (Apple Git-33)