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 <dem...@pivotal.io> AuthorDate: Fri Dec 20 18:18:54 2019 -0800 GEODE-7592: Simplify startManager() precondition checks (#4510) Co-authored-by: Dale Emery <dem...@pivotal.io> Co-authored-by: Joris Melchior <joris.melch...@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); + } +}