From dadadfb7493ba16b4278f0df8dc0ce0c5757edfc Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Fri, 6 Sep 2019 17:20:06 -0400 Subject: [PATCH] HBASE-22985 Gracefully ignore invalid MetricRegistries implementations If we happen to have a JAR on the classpath which advertises a bogus implementation of MetricRegistries, ignore it and don't crash. --- .../hbase/metrics/MetricRegistriesLoader.java | 23 +++++++++--- .../metrics/TestMetricRegistriesLoader.java | 36 +++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java b/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java index e17a28a582..d4d5b8620f 100644 --- a/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java +++ b/hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistriesLoader.java @@ -20,7 +20,9 @@ package org.apache.hadoop.hbase.metrics; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; +import java.util.ServiceConfigurationError; import java.util.ServiceLoader; import org.apache.hadoop.hbase.util.ReflectionUtils; @@ -47,7 +49,7 @@ public final class MetricRegistriesLoader { * @return A {@link MetricRegistries} implementation. */ public static MetricRegistries load() { - List availableImplementations = getDefinedImplemantations(); + List availableImplementations = getDefinedImplementations(); return load(availableImplementations); } @@ -86,11 +88,24 @@ public final class MetricRegistriesLoader { } } - private static List getDefinedImplemantations() { + private static List getDefinedImplementations() { ServiceLoader loader = ServiceLoader.load(MetricRegistries.class); + return loadDefinedImplementations(loader.iterator()); + } + + @VisibleForTesting + static List loadDefinedImplementations(Iterator iter) { List availableFactories = new ArrayList<>(); - for (MetricRegistries impl : loader) { - availableFactories.add(impl); + while (iter.hasNext()) { + try { + availableFactories.add(iter.next()); + } catch (ServiceConfigurationError e) { + // Should never happen unless a bad JAR is on the classpath. + // e.g. META-INF/services entry references impl that doesn't implement MetricsRegistries + // or referred implementation class is missing from the jar. + LOG.error("Found bad ServiceLoader entry on classpath (e.g. wrong interface, class missing)" + + ", ignoring.", e); + } } return availableFactories; } diff --git a/hbase-metrics-api/src/test/java/org/apache/hadoop/hbase/metrics/TestMetricRegistriesLoader.java b/hbase-metrics-api/src/test/java/org/apache/hadoop/hbase/metrics/TestMetricRegistriesLoader.java index 59f26999bd..9deedf1886 100644 --- a/hbase-metrics-api/src/test/java/org/apache/hadoop/hbase/metrics/TestMetricRegistriesLoader.java +++ b/hbase-metrics-api/src/test/java/org/apache/hadoop/hbase/metrics/TestMetricRegistriesLoader.java @@ -21,6 +21,11 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.mockito.Mockito.mock; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.ServiceConfigurationError; + import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.ClassRule; @@ -59,4 +64,35 @@ public class TestMetricRegistriesLoader { assertNotEquals(loader2, instance); assertNotEquals(loader3, instance); } + + @Test + public void testWrongRegistriesImplementation() { + final MetricRegistries mockRegistries = mock(MetricRegistries.class); + // Stub out an iterator which: + // first: returns a MetricRegistries + // second: throw ServiceConfigurationError + // third: throws NoSuchElementException + Iterator iter = new Iterator() { + int iterations = 0; + + @Override public boolean hasNext() { + return iterations < 2; + } + + @Override public MetricRegistries next() { + if (iterations == 0) { + iterations++; + return mockRegistries; + } else if (iterations == 1) { + iterations++; + throw new ServiceConfigurationError("Fail!"); + } + throw new NoSuchElementException(); + } + }; + + List foundImpls = MetricRegistriesLoader.loadDefinedImplementations(iter); + assertEquals(1, foundImpls.size()); + assertEquals(mockRegistries, foundImpls.get(0)); + } } -- 2.18.0