You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by kl...@apache.org on 2019/04/09 16:33:42 UTC
[geode] branch develop updated: GEODE-6609: Protect from
MetricsPublishingService exceptions (#3429)
This is an automated email from the ASF dual-hosted git repository.
klund pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 9f309df GEODE-6609: Protect from MetricsPublishingService exceptions (#3429)
9f309df is described below
commit 9f309df74ba2d5712a5e8767d839fad23dc12f81
Author: Kirk Lund <kl...@apache.org>
AuthorDate: Tue Apr 9 09:33:22 2019 -0700
GEODE-6609: Protect from MetricsPublishingService exceptions (#3429)
Catch and log Errors and RuntimeExceptions thrown from instantiation
of or calls to MetricsPublishingService implementations.
Co-authored-by: Michael Oleske <mo...@pivotal.io>
---
.../metrics/CacheLifecycleMetricsSession.java | 41 ++++++++-
.../internal/util/CollectingServiceLoader.java | 5 +-
.../internal/util/ListCollectingServiceLoader.java | 59 +++++++++++-
.../metrics/CacheLifecycleMetricsSessionTest.java | 77 ++++++++++++++++
.../util/ListCollectingServiceLoaderTest.java | 101 +++++++++++++++++++++
5 files changed, 274 insertions(+), 9 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSession.java b/geode-core/src/main/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSession.java
index 250616d..5ac029d 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSession.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSession.java
@@ -19,20 +19,26 @@ import java.util.HashSet;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.composite.CompositeMeterRegistry;
+import org.apache.logging.log4j.Logger;
+import org.apache.geode.SystemFailure;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.internal.cache.CacheLifecycleListener;
import org.apache.geode.internal.cache.GemFireCacheImpl;
import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.logging.LogService;
import org.apache.geode.internal.util.CollectingServiceLoader;
import org.apache.geode.internal.util.ListCollectingServiceLoader;
import org.apache.geode.metrics.MetricsPublishingService;
import org.apache.geode.metrics.MetricsSession;
public class CacheLifecycleMetricsSession implements MetricsSession, CacheLifecycleListener {
+ private static final Logger logger = LogService.getLogger();
+
private final CacheLifecycle cacheLifecycle;
private final CompositeMeterRegistry registry;
private final Collection<MetricsPublishingService> metricsPublishingServices;
+ private final ErrorLogger errorLogger;
public static Builder builder() {
return new Builder();
@@ -41,9 +47,16 @@ public class CacheLifecycleMetricsSession implements MetricsSession, CacheLifecy
@VisibleForTesting
CacheLifecycleMetricsSession(CacheLifecycle cacheLifecycle, CompositeMeterRegistry registry,
Collection<MetricsPublishingService> metricsPublishingServices) {
+ this(cacheLifecycle, registry, metricsPublishingServices, logger::error);
+ }
+
+ @VisibleForTesting
+ CacheLifecycleMetricsSession(CacheLifecycle cacheLifecycle, CompositeMeterRegistry registry,
+ Collection<MetricsPublishingService> metricsPublishingServices, ErrorLogger errorLogger) {
this.cacheLifecycle = cacheLifecycle;
this.registry = registry;
this.metricsPublishingServices = metricsPublishingServices;
+ this.errorLogger = errorLogger;
}
@Override
@@ -59,7 +72,14 @@ public class CacheLifecycleMetricsSession implements MetricsSession, CacheLifecy
@Override
public void cacheCreated(InternalCache cache) {
for (MetricsPublishingService metricsPublishingService : metricsPublishingServices) {
- metricsPublishingService.start(this);
+ try {
+ metricsPublishingService.start(this);
+ } catch (VirtualMachineError e) {
+ SystemFailure.initiateFailure(e);
+ throw e;
+ } catch (Error | RuntimeException e) {
+ logError(errorLogger, "start", metricsPublishingService.getClass().getName(), e);
+ }
}
}
@@ -68,7 +88,14 @@ public class CacheLifecycleMetricsSession implements MetricsSession, CacheLifecy
cacheLifecycle.removeListener(this);
for (MetricsPublishingService metricsPublishingService : metricsPublishingServices) {
- metricsPublishingService.stop();
+ try {
+ metricsPublishingService.stop();
+ } catch (VirtualMachineError e) {
+ SystemFailure.initiateFailure(e);
+ throw e;
+ } catch (Error | RuntimeException e) {
+ logError(errorLogger, "stop", metricsPublishingService.getClass().getName(), e);
+ }
}
for (MeterRegistry downstream : new HashSet<>(registry.getRegistries())) {
@@ -86,6 +113,16 @@ public class CacheLifecycleMetricsSession implements MetricsSession, CacheLifecy
return metricsPublishingServices;
}
+ private static void logError(ErrorLogger errorLogger, String methodName, String className,
+ Throwable throwable) {
+ errorLogger.logError("Error invoking {} for MetricsPublishingService implementation {}",
+ methodName, className, throwable);
+ }
+
+ interface ErrorLogger {
+ void logError(String message, String methodName, String className, Throwable throwable);
+ }
+
public static class Builder {
private CollectingServiceLoader serviceLoader = new ListCollectingServiceLoader();
diff --git a/geode-core/src/main/java/org/apache/geode/internal/util/CollectingServiceLoader.java b/geode-core/src/main/java/org/apache/geode/internal/util/CollectingServiceLoader.java
index 0b98c3b..5690e02 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/util/CollectingServiceLoader.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/util/CollectingServiceLoader.java
@@ -23,15 +23,14 @@ import java.util.ServiceLoader;
* Loads and returns a collection of all currently loadable implementations of the given service
* interface using {@link ServiceLoader}.
*/
-public interface CollectingServiceLoader {
+public interface CollectingServiceLoader<S> {
/**
* Loads and returns a collection of all currently loadable implementations of the given service
* interface.
*
* @param service the service interface
- * @param <S> the class of the service interface to load implementations of
* @return a collection of instantiated implementations of the given service interface
*/
- <S> Collection<S> loadServices(Class<S> service);
+ Collection<S> loadServices(Class<S> service);
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/util/ListCollectingServiceLoader.java b/geode-core/src/main/java/org/apache/geode/internal/util/ListCollectingServiceLoader.java
index 03fc697..b548677 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/util/ListCollectingServiceLoader.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/util/ListCollectingServiceLoader.java
@@ -18,19 +18,70 @@ package org.apache.geode.internal.util;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Iterator;
import java.util.List;
+import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.internal.logging.LogService;
+
/**
* Implements {@link CollectingServiceLoader} by returning a {@link List} of all currently loadable
* implementations of the given service interface.
*/
-public class ListCollectingServiceLoader implements CollectingServiceLoader {
+public class ListCollectingServiceLoader<S> implements CollectingServiceLoader<S> {
+ private static final Logger logger = LogService.getLogger();
+
+ private final ServiceLoaderWrapper<S> serviceLoaderWrapper;
+
+ public ListCollectingServiceLoader() {
+ this(new DefaultServiceLoader<>());
+ }
+
+ @VisibleForTesting
+ ListCollectingServiceLoader(ServiceLoaderWrapper<S> serviceLoaderWrapper) {
+ this.serviceLoaderWrapper = serviceLoaderWrapper;
+ }
@Override
- public <S> Collection<S> loadServices(Class<S> service) {
- List<S> services = new ArrayList<>();
- ServiceLoader.load(service).iterator().forEachRemaining(services::add);
+ public Collection<S> loadServices(Class<S> service) {
+ serviceLoaderWrapper.load(service);
+
+ Collection<S> services = new ArrayList<>();
+ for (Iterator<S> iterator = serviceLoaderWrapper.iterator(); iterator.hasNext();) {
+ try {
+ S instance = iterator.next();
+ services.add(instance);
+ } catch (ServiceConfigurationError serviceConfigurationError) {
+ logger.error("Error while loading implementations of {}", service.getName(),
+ serviceConfigurationError);
+ }
+ }
+
return services;
}
+
+ interface ServiceLoaderWrapper<S> {
+ void load(Class<S> service);
+
+ Iterator<S> iterator() throws ServiceConfigurationError;
+ }
+
+ private static class DefaultServiceLoader<S> implements ServiceLoaderWrapper<S> {
+
+ private ServiceLoader<S> actualServiceLoader;
+
+ @Override
+ public void load(Class<S> service) {
+ actualServiceLoader = ServiceLoader.load(service);
+ }
+
+ @Override
+ public Iterator<S> iterator() throws ServiceConfigurationError {
+ return actualServiceLoader.iterator();
+ }
+ }
}
diff --git a/geode-core/src/test/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSessionTest.java b/geode-core/src/test/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSessionTest.java
index 4a9af35..d20b743 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSessionTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/metrics/CacheLifecycleMetricsSessionTest.java
@@ -15,7 +15,11 @@
package org.apache.geode.internal.metrics;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
@@ -34,6 +38,7 @@ import org.junit.Test;
import org.apache.geode.internal.cache.GemFireCacheImpl;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.metrics.CacheLifecycleMetricsSession.CacheLifecycle;
+import org.apache.geode.internal.metrics.CacheLifecycleMetricsSession.ErrorLogger;
import org.apache.geode.metrics.MetricsPublishingService;
public class CacheLifecycleMetricsSessionTest {
@@ -214,6 +219,78 @@ public class CacheLifecycleMetricsSessionTest {
verify(theCacheLifecycle).removeListener(same(metricsSession));
}
+ @Test
+ public void cacheCreated_logsErrorMessage_ifMetricsPublishingServiceStartThrowsRuntimeException() {
+ MetricsPublishingService metricsPublishingService =
+ metricsPublishingService("metricsPublishingService");
+ String theClassName = metricsPublishingService.getClass().getName();
+ RuntimeException theException = new RuntimeException("theExceptionMessage");
+ doThrow(theException).when(metricsPublishingService).start(any());
+ List<MetricsPublishingService> metricsPublishingServices =
+ Collections.singletonList(metricsPublishingService);
+ ErrorLogger errorLogger = mock(ErrorLogger.class);
+ metricsSession = new CacheLifecycleMetricsSession(mock(CacheLifecycle.class), compositeRegistry,
+ metricsPublishingServices, errorLogger);
+
+ metricsSession.cacheCreated(mock(InternalCache.class));
+
+ verify(errorLogger).logError(anyString(), eq("start"), same(theClassName), same(theException));
+ }
+
+ @Test
+ public void cacheCreated_logsErrorMessage_ifMetricsPublishingServiceStartThrowsError() {
+ MetricsPublishingService metricsPublishingService =
+ metricsPublishingService("metricsPublishingService");
+ String theClassName = metricsPublishingService.getClass().getName();
+ Error theError = new Error("theErrorMessage");
+ doThrow(theError).when(metricsPublishingService).start(any());
+ List<MetricsPublishingService> metricsPublishingServices =
+ Collections.singletonList(metricsPublishingService);
+ ErrorLogger errorLogger = mock(ErrorLogger.class);
+ metricsSession = new CacheLifecycleMetricsSession(mock(CacheLifecycle.class), compositeRegistry,
+ metricsPublishingServices, errorLogger);
+
+ metricsSession.cacheCreated(mock(InternalCache.class));
+
+ verify(errorLogger).logError(anyString(), eq("start"), same(theClassName), same(theError));
+ }
+
+ @Test
+ public void cacheClosed_logsErrorMessage_ifMetricsPublishingServiceStopThrowsRuntimeException() {
+ MetricsPublishingService metricsPublishingService =
+ metricsPublishingService("metricsPublishingService");
+ String theClassName = metricsPublishingService.getClass().getName();
+ RuntimeException theException = new RuntimeException("theExceptionMessage");
+ doThrow(theException).when(metricsPublishingService).stop();
+ List<MetricsPublishingService> metricsPublishingServices =
+ Collections.singletonList(metricsPublishingService);
+ ErrorLogger errorLogger = mock(ErrorLogger.class);
+ metricsSession = new CacheLifecycleMetricsSession(mock(CacheLifecycle.class), compositeRegistry,
+ metricsPublishingServices, errorLogger);
+
+ metricsSession.cacheClosed(mock(InternalCache.class));
+
+ verify(errorLogger).logError(anyString(), eq("stop"), same(theClassName), same(theException));
+ }
+
+ @Test
+ public void cacheClosed_logsErrorMessage_ifMetricsPublishingServiceStopThrowsError() {
+ MetricsPublishingService metricsPublishingService =
+ metricsPublishingService("metricsPublishingService");
+ String theClassName = metricsPublishingService.getClass().getName();
+ Error theError = new Error("theErrorMessage");
+ doThrow(theError).when(metricsPublishingService).stop();
+ List<MetricsPublishingService> metricsPublishingServices =
+ Collections.singletonList(metricsPublishingService);
+ ErrorLogger errorLogger = mock(ErrorLogger.class);
+ metricsSession = new CacheLifecycleMetricsSession(mock(CacheLifecycle.class), compositeRegistry,
+ metricsPublishingServices, errorLogger);
+
+ metricsSession.cacheClosed(mock(InternalCache.class));
+
+ verify(errorLogger).logError(anyString(), eq("stop"), same(theClassName), same(theError));
+ }
+
private MetricsPublishingService metricsPublishingService(String name) {
return mock(MetricsPublishingService.class, name);
}
diff --git a/geode-core/src/test/java/org/apache/geode/internal/util/ListCollectingServiceLoaderTest.java b/geode-core/src/test/java/org/apache/geode/internal/util/ListCollectingServiceLoaderTest.java
new file mode 100644
index 0000000..0c55923
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/util/ListCollectingServiceLoaderTest.java
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.util;
+
+
+import static java.util.Arrays.asList;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.ServiceConfigurationError;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.InOrder;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import org.apache.geode.internal.util.ListCollectingServiceLoader.ServiceLoaderWrapper;
+import org.apache.geode.metrics.MetricsPublishingService;
+
+public class ListCollectingServiceLoaderTest {
+ @Rule
+ public MockitoRule rule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
+
+ @Mock
+ private ServiceLoaderWrapper<MetricsPublishingService> serviceLoaderWrapper;
+
+ @Test
+ public void loadServices_delegatesLoading() {
+ when(serviceLoaderWrapper.iterator()).thenReturn(mock(Iterator.class));
+
+ ListCollectingServiceLoader<MetricsPublishingService> collectingServiceLoader =
+ new ListCollectingServiceLoader<>(serviceLoaderWrapper);
+
+ collectingServiceLoader.loadServices(MetricsPublishingService.class);
+
+ InOrder inOrder = inOrder(serviceLoaderWrapper);
+ inOrder.verify(serviceLoaderWrapper).load(same(MetricsPublishingService.class));
+ inOrder.verify(serviceLoaderWrapper).iterator();
+ }
+
+ @Test
+ public void loadServices_returnsLoadedServices() {
+ MetricsPublishingService service1 = mock(MetricsPublishingService.class);
+ MetricsPublishingService service2 = mock(MetricsPublishingService.class);
+ MetricsPublishingService service3 = mock(MetricsPublishingService.class);
+ List<MetricsPublishingService> expectedServices = asList(service1, service2, service3);
+
+ when(serviceLoaderWrapper.iterator()).thenReturn(expectedServices.iterator());
+
+ ListCollectingServiceLoader<MetricsPublishingService> collectingServiceLoader =
+ new ListCollectingServiceLoader<>(serviceLoaderWrapper);
+
+ Collection<MetricsPublishingService> actualServices =
+ collectingServiceLoader.loadServices(MetricsPublishingService.class);
+
+ assertThat(actualServices)
+ .containsExactlyInAnyOrder(expectedServices.toArray(new MetricsPublishingService[0]));
+ }
+
+ @Test
+ public void loadServices_returnsLoadedServices_whenOneServiceThrows() {
+ MetricsPublishingService service1 = mock(MetricsPublishingService.class);
+ MetricsPublishingService service3 = mock(MetricsPublishingService.class);
+ Iterator<MetricsPublishingService> iterator = mock(Iterator.class);
+ ServiceConfigurationError error = new ServiceConfigurationError("Error message");
+
+ when(iterator.hasNext()).thenReturn(true, true, true, false);
+ when(iterator.next()).thenReturn(service1).thenThrow(error).thenReturn(service3);
+ when(serviceLoaderWrapper.iterator()).thenReturn(iterator);
+
+ ListCollectingServiceLoader<MetricsPublishingService> collectingServiceLoader =
+ new ListCollectingServiceLoader<>(serviceLoaderWrapper);
+
+ Collection<MetricsPublishingService> actualServices =
+ collectingServiceLoader.loadServices(MetricsPublishingService.class);
+
+ assertThat(actualServices)
+ .containsExactlyInAnyOrder(service1, service3);
+ }
+}