This is an automated email from the ASF dual-hosted git repository. jasonhuynh pushed a commit to branch support/1.13 in repository https://gitbox.apache.org/repos/asf/geode.git
commit dc547e9d2cbb6b5c399b7e68d0b1338d87e16bbf Author: Jinmei Liao <[email protected]> AuthorDate: Fri May 15 08:09:19 2020 -0700 GEODE-8078: log and report error at the correct place. (#5111) * For get operation, if cache is not ready, just don't report any runtime information * do not log the CacheClosedException (cherry picked from commit a235c7c4ea2474d6fe347326ef888c7c10daffb6) --- .../api/LocatorClusterManagementService.java | 4 +- .../functions/CacheRealizationFunction.java | 47 ++++++-- .../functions/CacheRealizationFunctionTest.java | 129 +++++++++++++++++++++ .../management/client/GetStartingMemberTest.java | 93 +++++++++++++++ .../MemberManagementServiceRestDUnitTest.java} | 6 +- 5 files changed, 264 insertions(+), 15 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java b/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java index 5256359..8561a20 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java @@ -29,6 +29,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; @@ -575,7 +576,8 @@ public class LocatorClusterManagementService implements ClusterManagementService .setArguments(Arrays.asList(configuration, operation, null)); ((AbstractExecution) execution).setIgnoreDepartedMembers(true); ResultCollector rc = execution.execute(function); - return (List<R>) rc.getResult(); + return ((List<R>) rc.getResult()).stream().filter(Objects::nonNull) + .collect(Collectors.toList()); } // if we have file arguments, we need to export the file input stream for each member diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/functions/CacheRealizationFunction.java b/geode-core/src/main/java/org/apache/geode/management/internal/functions/CacheRealizationFunction.java index c82ad90..3e8733a 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/functions/CacheRealizationFunction.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/functions/CacheRealizationFunction.java @@ -36,6 +36,8 @@ import org.apache.commons.io.IOUtils; import org.apache.logging.log4j.Logger; import org.apache.geode.annotations.Immutable; +import org.apache.geode.annotations.VisibleForTesting; +import org.apache.geode.cache.CacheClosedException; import org.apache.geode.cache.execute.FunctionContext; import org.apache.geode.internal.cache.InternalCache; import org.apache.geode.internal.cache.execute.InternalFunction; @@ -81,24 +83,45 @@ public class CacheRealizationFunction implements InternalFunction<List> { public void execute(FunctionContext<List> context) { AbstractConfiguration cacheElement = (AbstractConfiguration) context.getArguments().get(0); CacheElementOperation operation = (CacheElementOperation) context.getArguments().get(1); - RemoteInputStream jarStream = (RemoteInputStream) context.getArguments().get(2); - InternalCache cache = (InternalCache) context.getCache(); - try { - if (operation == CacheElementOperation.GET) { + // for get operation, caller is expecting RuntimeInfo + if (operation == CacheElementOperation.GET) { + try { + InternalCache cache = (InternalCache) context.getCache(); context.getResultSender().lastResult(executeGet(context, cache, cacheElement)); - } else { + } catch (CacheClosedException e) { + // cache not ready or closed already, no need to log and do not return any runtime info + context.getResultSender().lastResult(null); + } catch (Exception e) { + logError("Unable to gather runtime information on this member. ", e); + context.getResultSender().lastResult(null); + } + } + // for other operations, caller is expecting RealizationResult + else { + try { + RemoteInputStream jarStream = (RemoteInputStream) context.getArguments().get(2); + InternalCache cache = (InternalCache) context.getCache(); context.getResultSender() .lastResult(executeUpdate(context, cache, cacheElement, operation, jarStream)); + } catch (CacheClosedException e) { + // cache not ready or closed already, no need to log it + context.getResultSender().lastResult(new RealizationResult() + .setSuccess(false) + .setMessage(e.getMessage())); + } catch (Exception e) { + logError("unable to update cache with the configuration.", e); + context.getResultSender().lastResult(new RealizationResult() + .setSuccess(false) + .setMemberName(context.getMemberName()) + .setMessage(e.getMessage())); } - } catch (Exception e) { - logger.error(e.getMessage(), e); - context.getResultSender().lastResult(new RealizationResult() - .setSuccess(false) - .setMemberName(context.getMemberName()) - .setMessage(e.getMessage())); } + } + @VisibleForTesting + void logError(String s, Exception e) { + logger.error(s, e); } public RuntimeInfo executeGet(FunctionContext<List> context, @@ -110,7 +133,7 @@ public class CacheRealizationFunction implements InternalFunction<List> { } RuntimeInfo runtimeInfo = realizer.get(cacheElement, cache); - // set the membername if this is not a global runtime + // set the member name if this is not a global runtime if (!cacheElement.isGlobalRuntime()) { runtimeInfo.setMemberName(context.getMemberName()); } diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/functions/CacheRealizationFunctionTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/functions/CacheRealizationFunctionTest.java new file mode 100644 index 0000000..d942b29 --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/management/internal/functions/CacheRealizationFunctionTest.java @@ -0,0 +1,129 @@ +/* + * 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.functions; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; + +import com.healthmarketscience.rmiio.RemoteInputStream; +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import org.apache.geode.cache.CacheClosedException; +import org.apache.geode.cache.execute.FunctionContext; +import org.apache.geode.cache.execute.ResultSender; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.management.api.RealizationResult; +import org.apache.geode.management.configuration.AbstractConfiguration; +import org.apache.geode.management.internal.CacheElementOperation; + +public class CacheRealizationFunctionTest { + + private CacheRealizationFunction function; + private FunctionContext<List> context; + private AbstractConfiguration config; + private CacheElementOperation operation; + private InternalCache cache; + private List arguments; + private ResultSender resultSender; + private RemoteInputStream inputStream; + + @Before + public void before() throws Exception { + function = spy(CacheRealizationFunction.class); + context = mock(FunctionContext.class); + when(context.getMemberName()).thenReturn("testName"); + arguments = new ArrayList(); + when(context.getArguments()).thenReturn(arguments); + config = mock(AbstractConfiguration.class); + arguments.add(config); + resultSender = mock(ResultSender.class); + when(context.getResultSender()).thenReturn(resultSender); + cache = mock(InternalCache.class); + inputStream = mock(RemoteInputStream.class); + } + + @Test + public void GetWithCacheClosed() throws Exception { + operation = CacheElementOperation.GET; + arguments.add(operation); + when(context.getCache()).thenThrow(CacheClosedException.class); + + function.execute(context); + verify(function, never()).logError(any(), any()); + verify(resultSender).lastResult(null); + } + + @Test + public void GetWithOtherException() throws Exception { + operation = CacheElementOperation.GET; + arguments.add(operation); + when(context.getCache()).thenReturn(cache); + doThrow(RuntimeException.class).when(function).executeGet(context, cache, config); + + function.execute(context); + verify(function).logError(any(), any()); + verify(resultSender).lastResult(null); + } + + @Test + public void CreateWithCacheClosed() throws Exception { + operation = CacheElementOperation.CREATE; + arguments.add(operation); + arguments.add(inputStream); + when(context.getCache()).thenThrow(CacheClosedException.class); + + function.execute(context); + verify(function, never()).logError(any(), any()); + ArgumentCaptor<RealizationResult> argumentCaptor = + ArgumentCaptor.forClass(RealizationResult.class); + verify(resultSender).lastResult(argumentCaptor.capture()); + RealizationResult result = argumentCaptor.getValue(); + // since cache is already closed, no way to get the membername + // in the result + assertThat(result.getMemberName()).isNull(); + } + + @Test + public void CreateWithOtherException() throws Exception { + operation = CacheElementOperation.CREATE; + arguments.add(operation); + arguments.add(inputStream); + when(context.getCache()).thenReturn(cache); + + doThrow(RuntimeException.class).when(function).executeUpdate(context, cache, config, operation, + inputStream); + + function.execute(context); + verify(function).logError(any(), any()); + ArgumentCaptor<RealizationResult> argumentCaptor = + ArgumentCaptor.forClass(RealizationResult.class); + verify(resultSender).lastResult(argumentCaptor.capture()); + RealizationResult result = argumentCaptor.getValue(); + assertThat(result.getMemberName()).isEqualTo("testName"); + } + +} diff --git a/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/GetStartingMemberTest.java b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/GetStartingMemberTest.java new file mode 100644 index 0000000..8fbd3bf --- /dev/null +++ b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/GetStartingMemberTest.java @@ -0,0 +1,93 @@ +/* + * 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.client; + + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.apache.geode.test.awaitility.GeodeAwaitility.await; +import static org.apache.geode.test.awaitility.GeodeAwaitility.getTimeout; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.Future; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.context.web.WebAppConfiguration; +import org.springframework.web.client.RestTemplate; +import org.springframework.web.context.WebApplicationContext; + +import org.apache.geode.management.api.ClusterManagementException; +import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.api.RestTemplateClusterManagementServiceTransport; +import org.apache.geode.management.configuration.Member; +import org.apache.geode.management.internal.rest.LocatorWebContext; +import org.apache.geode.management.internal.rest.PlainLocatorContextLoader; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.junit.rules.ExecutorServiceRule; + +@RunWith(SpringRunner.class) +@ContextConfiguration(locations = {"classpath*:WEB-INF/management-servlet.xml"}, + loader = PlainLocatorContextLoader.class) +@WebAppConfiguration +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) +public class GetStartingMemberTest { + + @Autowired + private WebApplicationContext webApplicationContext; + + @Rule + public ClusterStartupRule cluster = new ClusterStartupRule(1); + + @Rule + public ExecutorServiceRule executorServiceRule = new ExecutorServiceRule(); + + private ClusterManagementService client; + private LocatorWebContext webContext; + + @Before + public void before() { + cluster.setSkipLocalDistributedSystemCleanup(true); + webContext = new LocatorWebContext(webApplicationContext); + client = new ClusterManagementServiceBuilder().setTransport( + new RestTemplateClusterManagementServiceTransport( + new RestTemplate(webContext.getRequestFactory()))) + .build(); + } + + @Test + public void getStartingMember() throws Exception { + Member server0 = new Member(); + server0.setId("server-0"); + + Future<Void> startServer = executorServiceRule.submit(() -> { + cluster.startServerVM(0, webContext.getLocator().getPort()); + }); + + await() + .ignoreException(ClusterManagementException.class) + .untilAsserted(() -> assertThat(client.get(server0).getStatusCode()).isEqualTo( + ClusterManagementResult.StatusCode.OK)); + + startServer.get(getTimeout().toMillis(), MILLISECONDS); + } +} diff --git a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceRestIntegrationTest.java b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceRestDUnitTest.java similarity index 96% rename from geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceRestIntegrationTest.java rename to geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceRestDUnitTest.java index d5fefaa..628de8a 100644 --- a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceRestIntegrationTest.java +++ b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceRestDUnitTest.java @@ -13,7 +13,7 @@ * the License. */ -package org.apache.geode.management.internal.rest; +package org.apache.geode.management.client; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -38,6 +38,8 @@ import org.springframework.test.context.junit4.SpringRunner; import org.springframework.test.context.web.WebAppConfiguration; import org.springframework.web.context.WebApplicationContext; +import org.apache.geode.management.internal.rest.LocatorLauncherContextLoader; +import org.apache.geode.management.internal.rest.LocatorWebContext; import org.apache.geode.test.dunit.rules.ClusterStartupRule; @RunWith(SpringRunner.class) @@ -45,7 +47,7 @@ import org.apache.geode.test.dunit.rules.ClusterStartupRule; loader = LocatorLauncherContextLoader.class) @WebAppConfiguration @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) -public class MemberManagementServiceRestIntegrationTest { +public class MemberManagementServiceRestDUnitTest { @Autowired private WebApplicationContext webApplicationContext;
