You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ji...@apache.org on 2019/12/21 02:19:19 UTC
[geode] branch develop updated: GEODE-7592: Simplify startManager()
precondition checks (#4510)
This is an automated email from the ASF dual-hosted git repository.
jinmeiliao 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 5ec5dd1 GEODE-7592: Simplify startManager() precondition checks (#4510)
5ec5dd1 is described below
commit 5ec5dd182ac8f523ee83518b4d727a5527203f5a
Author: Dale Emery <de...@pivotal.io>
AuthorDate: Fri Dec 20 18:18:54 2019 -0800
GEODE-7592: Simplify startManager() precondition checks (#4510)
Co-authored-by: Dale Emery <de...@pivotal.io>
Co-authored-by: Joris Melchior <jo...@gmail.com>
* LGTM complained about a possible NPE in startManager(). There was no
possibility of an NPE, but precondition-checking code was overly
complex, and difficult for LGTM and humans to analyze.
* Adding the tests required injecting several dependencies.
---
.../internal/SystemManagementService.java | 83 +++++---
.../internal/SystemManagementServiceTest.java | 218 +++++++++++++++++++++
2 files changed, 271 insertions(+), 30 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java b/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java
index cb00c70..2e9403e 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java
@@ -25,6 +25,8 @@ import java.util.List;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ExecutorService;
+import java.util.function.BiFunction;
+import java.util.function.Function;
import java.util.function.Supplier;
import javax.management.Notification;
@@ -39,6 +41,7 @@ import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.cache.execute.FunctionService;
import org.apache.geode.distributed.DistributedMember;
import org.apache.geode.distributed.DistributedSystemDisconnectedException;
+import org.apache.geode.distributed.internal.DistributionConfig;
import org.apache.geode.distributed.internal.InternalDistributedSystem;
import org.apache.geode.distributed.internal.ResourceEvent;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
@@ -113,6 +116,7 @@ public class SystemManagementService extends BaseManagementService {
private final StatisticsClock statisticsClock;
private final FederatingManagerFactory federatingManagerFactory;
+
/**
* whether the service is closed or not if cache is closed automatically this service will be
* closed
@@ -133,15 +137,33 @@ public class SystemManagementService extends BaseManagementService {
* Managing node.
*/
private ManagementMembershipListener listener;
+ private final Function<SystemManagementService, LocalManager> localManagerFactory;
static BaseManagementService newSystemManagementService(
InternalCacheForClientAccess cache) {
- return new SystemManagementService(cache).init();
+ return newSystemManagementService(cache, NotificationHub::new,
+ SystemManagementService::createLocalManager,
+ createFederatingManagerFactory(), ManagementAgent::new);
}
- private SystemManagementService(InternalCacheForClientAccess cache) {
+ @VisibleForTesting
+ static BaseManagementService newSystemManagementService(InternalCacheForClientAccess cache,
+ Function<ManagementResourceRepo, NotificationHub> notificationHubFactory,
+ Function<SystemManagementService, LocalManager> localManagerFactory,
+ FederatingManagerFactory federatingManagerFactory,
+ BiFunction<DistributionConfig, InternalCacheForClientAccess, ManagementAgent> managementAgentFactory) {
+ return new SystemManagementService(cache, notificationHubFactory, localManagerFactory,
+ federatingManagerFactory, managementAgentFactory).init();
+ }
+
+ private SystemManagementService(InternalCacheForClientAccess cache,
+ Function<ManagementResourceRepo, NotificationHub> notificationHubFactory,
+ Function<SystemManagementService, LocalManager> localManagerFactory,
+ FederatingManagerFactory federatingManagerFactory,
+ BiFunction<DistributionConfig, InternalCacheForClientAccess, ManagementAgent> managementAgentFactory) {
this.cache = cache;
system = cache.getInternalDistributedSystem();
+ this.localManagerFactory = localManagerFactory;
if (!system.isConnected()) {
throw new DistributedSystemDisconnectedException(
@@ -152,10 +174,10 @@ public class SystemManagementService extends BaseManagementService {
statisticsClock = cache.getStatisticsClock();
jmxAdapter = new MBeanJMXAdapter(system.getDistributedMember());
repo = new ManagementResourceRepo();
- notificationHub = new NotificationHub(repo);
+ notificationHub = notificationHubFactory.apply(repo);
if (system.getConfig().getJmxManager()) {
- agent = new ManagementAgent(system.getConfig(), cache);
+ agent = managementAgentFactory.apply(system.getConfig(), cache);
} else {
agent = null;
}
@@ -163,7 +185,7 @@ public class SystemManagementService extends BaseManagementService {
FunctionService.registerFunction(new ManagementFunction(notificationHub));
proxyListeners = new CopyOnWriteArrayList<>();
- federatingManagerFactory = createFederatingManagerFactory();
+ this.federatingManagerFactory = federatingManagerFactory;
}
@Override
@@ -341,34 +363,28 @@ public class SystemManagementService extends BaseManagementService {
"Manager is already running");
}
- boolean needsToBeStarted = false;
if (!isManagerCreated()) {
createManager();
- needsToBeStarted = true;
- } else if (!federatingManager.isRunning()) {
- needsToBeStarted = true;
}
- if (needsToBeStarted) {
- boolean started = false;
- try {
- system.handleResourceEvent(ResourceEvent.MANAGER_START, null);
- federatingManager.startManager();
- if (agent != null) {
- agent.startAgent();
- }
- cache.getJmxManagerAdvisor().broadcastChange();
- started = true;
- } catch (RuntimeException | Error e) {
- logger.error("Jmx manager could not be started because {}", e.getMessage(), e);
- throw e;
- } finally {
- if (!started) {
- if (federatingManager != null) {
- federatingManager.stopManager();
- }
- system.handleResourceEvent(ResourceEvent.MANAGER_STOP, null);
+ boolean started = false;
+ try {
+ system.handleResourceEvent(ResourceEvent.MANAGER_START, null);
+ federatingManager.startManager();
+ if (agent != null) {
+ agent.startAgent();
+ }
+ cache.getJmxManagerAdvisor().broadcastChange();
+ started = true;
+ } catch (RuntimeException | Error e) {
+ logger.error("Jmx manager could not be started because {}", e.getMessage(), e);
+ throw e;
+ } finally {
+ if (!started) {
+ if (federatingManager != null) {
+ federatingManager.stopManager();
}
+ system.handleResourceEvent(ResourceEvent.MANAGER_STOP, null);
}
}
}
@@ -672,8 +688,7 @@ public class SystemManagementService extends BaseManagementService {
*/
private SystemManagementService init() {
try {
- localManager =
- new LocalManager(repo, system, this, cache, statisticsFactory, statisticsClock);
+ localManager = localManagerFactory.apply(this);
listener = new ManagementMembershipListener(this);
localManager.startManager();
@@ -690,6 +705,14 @@ public class SystemManagementService extends BaseManagementService {
}
}
+ private static LocalManager createLocalManager(SystemManagementService service) {
+ return service.newLocalManager();
+ }
+
+ private LocalManager newLocalManager() {
+ return new LocalManager(repo, system, this, cache, statisticsFactory, statisticsClock);
+ }
+
private static FederatingManagerFactory createFederatingManagerFactory() {
try {
String federatingManagerFactoryName =
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/SystemManagementServiceTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/SystemManagementServiceTest.java
new file mode 100644
index 0000000..627a5b4
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/SystemManagementServiceTest.java
@@ -0,0 +1,218 @@
+/*
+ * 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.management.internal;
+
+import static org.apache.geode.distributed.internal.ResourceEvent.MANAGER_START;
+import static org.apache.geode.distributed.internal.ResourceEvent.MANAGER_STOP;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.clearInvocations;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+import static org.mockito.quality.Strictness.LENIENT;
+
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+
+import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.InternalCacheForClientAccess;
+import org.apache.geode.management.AlreadyRunningException;
+import org.apache.geode.management.ManagementException;
+
+public class SystemManagementServiceTest {
+
+ @Rule
+ public MockitoRule rule = MockitoJUnit.rule().strictness(LENIENT);
+
+ @Mock
+ private FederatingManagerFactory federatingManagerFactory;
+ @Mock
+ private InternalCacheForClientAccess cache;
+ @Mock
+ private DistributionConfig config;
+ @Mock
+ private FederatingManager federatingManager;
+ @Mock
+ private JmxManagerAdvisor jmxManagerAdvisor;
+ @Mock
+ private Function<SystemManagementService, LocalManager> localManagerFactory;
+ @Mock
+ private ManagementAgent managementAgent;
+ @Mock
+ private BiFunction<DistributionConfig, InternalCacheForClientAccess, ManagementAgent> managementAgentFactory;
+ @Mock
+ private Function<ManagementResourceRepo, NotificationHub> notificationHubFactory;
+ @Mock
+ private InternalDistributedSystem system;
+
+ @Before
+ public void setup() {
+ when(config.getJmxManager()).thenReturn(true);
+
+ when(system.isConnected()).thenReturn(true);
+ when(system.getConfig()).thenReturn(config);
+ when(system.getDistributionManager()).thenReturn(mock(DistributionManager.class));
+
+ when(cache.getInternalDistributedSystem()).thenReturn(system);
+ when(cache.getJmxManagerAdvisor()).thenReturn(jmxManagerAdvisor);
+
+ when(federatingManagerFactory
+ .create(any(), any(), any(), any(), any(), any(), any(), any(), any()))
+ .thenReturn(federatingManager);
+
+ when(managementAgentFactory.apply(any(), any())).thenReturn(managementAgent);
+ when(notificationHubFactory.apply(any())).thenReturn(mock(NotificationHub.class));
+ when(localManagerFactory.apply(any())).thenReturn(mock(LocalManager.class));
+ }
+
+ @Test
+ public void startManager_throws_ifIfNotWillingToBeJmxManager() {
+ when(config.getJmxManager()).thenReturn(false);
+
+ BaseManagementService service = systemManagementService();
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(ManagementException.class);
+ }
+
+ @Test
+ public void startManager_throws_ifSystemIsNotConnected() {
+ // Must be connected to construct the service
+ when(system.isConnected()).thenReturn(true);
+
+ BaseManagementService service = systemManagementService();
+
+ when(system.isConnected()).thenReturn(false);
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(ManagementException.class);
+ }
+
+ @Test
+ public void startManager_throws_ifServiceIsClosed() {
+ BaseManagementService service = systemManagementService();
+
+ service.close();
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(ManagementException.class);
+ }
+
+ @Test
+ public void startManager_throws_ifExistingFederatingManagerIsAlreadyRunning() {
+ BaseManagementService service = systemManagementService();
+
+ service.startManager();
+
+ when(federatingManager.isRunning()).thenReturn(true);
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(AlreadyRunningException.class);
+ }
+
+ @Test
+ public void startManager_startsExistingFederatingManager_ifNotAlreadyStarted() {
+ BaseManagementService service = systemManagementService();
+
+ service.startManager();
+
+ clearInvocations(federatingManager);
+ clearInvocations(federatingManagerFactory);
+
+ when(federatingManager.isRunning()).thenReturn(false);
+
+ service.startManager();
+
+ verify(federatingManager).startManager();
+
+ // Verify that the service did not create a second federating manager
+ verifyNoMoreInteractions(federatingManagerFactory);
+ }
+
+ @Test
+ public void startManager_startsNewFederatingManager_ifNoExistingFederatingManager() {
+ BaseManagementService service = systemManagementService();
+
+ service.startManager();
+
+ verify(federatingManagerFactory)
+ .create(any(), any(), any(), any(), any(), any(), any(), any(), any());
+ verify(federatingManager).startManager();
+ }
+
+ @Test
+ public void startManager_reportsManagerStarted() {
+ BaseManagementService service = systemManagementService();
+
+ service.startManager();
+
+ verify(system).handleResourceEvent(eq(MANAGER_START), any());
+ }
+
+ @Test
+ public void startManager_broadcastsJmxManagerChange() {
+ BaseManagementService service = systemManagementService();
+
+ service.startManager();
+
+ verify(jmxManagerAdvisor, atLeastOnce()).broadcastChange();
+ }
+
+ @Test
+ public void startManager_stopsFederatingManager_ifRuntimeExceptionAfterStarting() {
+ BaseManagementService service = systemManagementService();
+
+ // Called after starting federating manager
+ doThrow(new RuntimeException("thrown for testing")).when(managementAgent).startAgent();
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(RuntimeException.class);
+
+ verify(federatingManager).stopManager();
+ }
+
+ @Test
+ public void startManager_reportsManagerStopped_ifRuntimeExceptionAfterStarting() {
+ BaseManagementService service = systemManagementService();
+
+ // Called after starting federating manager
+ doThrow(new RuntimeException("thrown for testing")).when(managementAgent).startAgent();
+
+ assertThatThrownBy(service::startManager)
+ .isInstanceOf(RuntimeException.class);
+
+ verify(system).handleResourceEvent(eq(MANAGER_STOP), any());
+ }
+
+ private BaseManagementService systemManagementService() {
+ return SystemManagementService.newSystemManagementService(cache, notificationHubFactory,
+ localManagerFactory, federatingManagerFactory, managementAgentFactory);
+ }
+}