From 440ec19d54ade1d0c0a92d8477bf67b46d2cf567 Mon Sep 17 00:00:00 2001 From: jinmeiliao Date: Fri, 29 Mar 2019 15:17:02 -0700 Subject: [PATCH 1/8] GEODE-5971: refactor misc commands to use ResultModel (#3363) --- .../HistoryCommandIntegrationTest.java | 15 +-- .../internal/cli/commands/HistoryCommand.java | 106 ++++++------------ .../cli/commands/ListIndexCommand.java | 91 ++++++++------- .../cli/commands/PDXRenameCommand.java | 57 +++++----- 4 files changed, 112 insertions(+), 157 deletions(-) diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java index 88f0e7271b3f..a259bb91ccba 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/commands/HistoryCommandIntegrationTest.java @@ -47,13 +47,13 @@ public void tearDown() throws Exception { @Test public void testHistoryWithEntry() { // Generate a line of history - gfsh.executeCommand("echo --string=string"); - gfsh.executeCommand("connect"); + gfsh.executeAndAssertThat("echo --string=string"); + gfsh.executeAndAssertThat("connect"); gfsh.executeAndAssertThat("history").statusIsSuccess() - .containsOutput( - " 1 0: echo --string=string" + System.lineSeparator() + " 2 1: connect" + - System.lineSeparator() + System.lineSeparator() + System.lineSeparator()); + .hasInfoSection() + .hasLines() + .containsExactly("0: echo --string=string", "1: connect"); } @Test @@ -70,7 +70,8 @@ public void testHistoryWithFileName() throws IOException { assertThat(historyFile).doesNotExist(); String command = "history --file=" + historyFile.getAbsolutePath(); - gfsh.executeAndAssertThat(command).statusIsSuccess(); + gfsh.executeAndAssertThat(command).statusIsSuccess() + .containsOutput("Wrote successfully to file"); assertThat(historyFile).exists(); assertThat(historyFile).hasContent("0: echo --string=string"); @@ -79,7 +80,7 @@ public void testHistoryWithFileName() throws IOException { @Test public void testClearHistory() { // Generate a line of history - gfsh.executeCommand("echo --string=string"); + gfsh.executeAndAssertThat("echo --string=string"); gfsh.executeAndAssertThat("history --clear").statusIsSuccess() .containsOutput(CliStrings.HISTORY__MSG__CLEARED_HISTORY); diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/HistoryCommand.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/HistoryCommand.java index 0925ef2afbda..1475670e948e 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/HistoryCommand.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/HistoryCommand.java @@ -15,119 +15,81 @@ package org.apache.geode.management.internal.cli.commands; -import java.io.BufferedWriter; import java.io.File; -import java.io.FileWriter; import java.io.IOException; -import java.io.Writer; import java.util.Iterator; +import org.apache.commons.io.FileUtils; +import org.apache.commons.lang3.StringUtils; import org.springframework.shell.core.annotation.CliCommand; import org.springframework.shell.core.annotation.CliOption; import org.apache.geode.management.cli.CliMetaData; -import org.apache.geode.management.cli.Result; import org.apache.geode.management.internal.cli.GfshParser; import org.apache.geode.management.internal.cli.i18n.CliStrings; -import org.apache.geode.management.internal.cli.result.ErrorResultData; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.model.InfoResultModel; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.management.internal.cli.shell.Gfsh; import org.apache.geode.management.internal.cli.shell.jline.GfshHistory; -public class HistoryCommand extends InternalGfshCommand { +public class HistoryCommand extends OfflineGfshCommand { @CliCommand(value = CliStrings.HISTORY, help = CliStrings.HISTORY__HELP) @CliMetaData(shellOnly = true, relatedTopic = {CliStrings.TOPIC_GFSH}) - public Result history( + public ResultModel history( @CliOption(key = {CliStrings.HISTORY__FILE}, help = CliStrings.HISTORY__FILE__HELP) String saveHistoryTo, @CliOption(key = {CliStrings.HISTORY__CLEAR}, specifiedDefaultValue = "true", unspecifiedDefaultValue = "false", - help = CliStrings.HISTORY__CLEAR__HELP) Boolean clearHistory) { + help = CliStrings.HISTORY__CLEAR__HELP) Boolean clearHistory) + throws IOException { // process clear history if (clearHistory) { return executeClearHistory(); } else { // Process file option - Gfsh gfsh = Gfsh.getCurrentInstance(); - ErrorResultData errorResultData; - StringBuilder contents = new StringBuilder(); - Writer output = null; + Gfsh gfsh = getGfsh(); - int historySize = gfsh.getHistorySize(); - String historySizeString = String.valueOf(historySize); - int historySizeWordLength = historySizeString.length(); + boolean hasFile = StringUtils.isNotBlank(saveHistoryTo); + + File saveHistoryToFile = null; + if (hasFile) { + saveHistoryToFile = new File(saveHistoryTo); + if (saveHistoryToFile.exists()) { + return ResultModel.createError("File exists already"); + } + if (saveHistoryToFile.isDirectory()) { + return ResultModel.createError(CliStrings.HISTORY__MSG__FILE_SHOULD_NOT_BE_DIRECTORY); + } + } GfshHistory gfshHistory = gfsh.getGfshHistory(); Iterator it = gfshHistory.entries(); - boolean flagForLineNumbers = !(saveHistoryTo != null && saveHistoryTo.length() > 0); - long lineNumber = 0; + ResultModel result = new ResultModel(); + InfoResultModel histories = result.addInfo("history"); while (it.hasNext()) { String line = it.next().toString(); if (!line.isEmpty()) { - if (flagForLineNumbers) { - lineNumber++; - contents.append(String.format("%" + historySizeWordLength + "s ", lineNumber)); + if (hasFile) { + FileUtils.writeStringToFile(saveHistoryToFile, line + GfshParser.LINE_SEPARATOR, + "UTF-8", true); + } else { + histories.addLine(line); } - contents.append(line); - contents.append(GfshParser.LINE_SEPARATOR); } } - try { - // write to a user file - if (saveHistoryTo != null && saveHistoryTo.length() > 0) { - File saveHistoryToFile = new File(saveHistoryTo); - output = new BufferedWriter(new FileWriter(saveHistoryToFile)); - - if (!saveHistoryToFile.exists()) { - errorResultData = - ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT) - .addLine(CliStrings.HISTORY__MSG__FILE_DOES_NOT_EXISTS); - return ResultBuilder.buildResult(errorResultData); - } - if (!saveHistoryToFile.isFile()) { - errorResultData = - ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT) - .addLine(CliStrings.HISTORY__MSG__FILE_SHOULD_NOT_BE_DIRECTORY); - return ResultBuilder.buildResult(errorResultData); - } - if (!saveHistoryToFile.canWrite()) { - errorResultData = - ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT) - .addLine(CliStrings.HISTORY__MSG__FILE_CANNOT_BE_WRITTEN); - return ResultBuilder.buildResult(errorResultData); - } - - output.write(contents.toString()); - } - - } catch (IOException ex) { - return ResultBuilder - .createInfoResult("File error " + ex.getMessage() + " for file " + saveHistoryTo); - } finally { - try { - if (output != null) { - output.close(); - } - } catch (IOException e) { - errorResultData = ResultBuilder.createErrorResultData() - .setErrorCode(ResultBuilder.ERRORCODE_DEFAULT).addLine("exception in closing file"); - return ResultBuilder.buildResult(errorResultData); - } - } - if (saveHistoryTo != null && saveHistoryTo.length() > 0) { + if (hasFile) { // since written to file no need to display the content - return ResultBuilder.createInfoResult("Wrote successfully to file " + saveHistoryTo); + return ResultModel.createInfo("Wrote successfully to file " + saveHistoryTo); } else { - return ResultBuilder.createInfoResult(contents.toString()); + return result; } } } - private Result executeClearHistory() { - Gfsh gfsh = Gfsh.getCurrentInstance(); - gfsh.clearHistory(); - return ResultBuilder.createInfoResult(CliStrings.HISTORY__MSG__CLEARED_HISTORY); + private ResultModel executeClearHistory() { + getGfsh().clearHistory(); + return ResultModel.createInfo(CliStrings.HISTORY__MSG__CLEARED_HISTORY); } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListIndexCommand.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListIndexCommand.java index e3b598d1d731..64fa06621e4c 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListIndexCommand.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListIndexCommand.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.Set; import org.springframework.shell.core.annotation.CliCommand; @@ -28,64 +29,60 @@ import org.apache.geode.internal.cache.execute.AbstractExecution; import org.apache.geode.internal.lang.StringUtils; import org.apache.geode.management.cli.CliMetaData; -import org.apache.geode.management.cli.Result; +import org.apache.geode.management.cli.GfshCommand; import org.apache.geode.management.internal.cli.domain.IndexDetails; import org.apache.geode.management.internal.cli.domain.IndexDetails.IndexStatisticsDetails; import org.apache.geode.management.internal.cli.functions.ListIndexFunction; import org.apache.geode.management.internal.cli.i18n.CliStrings; -import org.apache.geode.management.internal.cli.result.ResultBuilder; -import org.apache.geode.management.internal.cli.result.TabularResultData; +import org.apache.geode.management.internal.cli.result.model.ResultModel; +import org.apache.geode.management.internal.cli.result.model.TabularResultModel; import org.apache.geode.management.internal.security.ResourceOperation; import org.apache.geode.security.ResourcePermission; -public class ListIndexCommand extends InternalGfshCommand { +public class ListIndexCommand extends GfshCommand { @CliCommand(value = CliStrings.LIST_INDEX, help = CliStrings.LIST_INDEX__HELP) @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_REGION, CliStrings.TOPIC_GEODE_DATA}) @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER, operation = ResourcePermission.Operation.READ, target = ResourcePermission.Target.QUERY) - public Result listIndex(@CliOption(key = CliStrings.LIST_INDEX__STATS, + public ResultModel listIndex(@CliOption(key = CliStrings.LIST_INDEX__STATS, specifiedDefaultValue = "true", unspecifiedDefaultValue = "false", help = CliStrings.LIST_INDEX__STATS__HELP) final boolean showStats) { - return toTabularResult(getIndexListing(), showStats); - } + ResultModel result = new ResultModel(); + TabularResultModel indexTable = result.addTable("indices"); + final List indexDetailsList = getIndexListing(); + if (indexDetailsList.isEmpty()) { + return ResultModel.createInfo(CliStrings.LIST_INDEX__INDEXES_NOT_FOUND_MESSAGE); + } - private Result toTabularResult(final List indexDetailsList, - final boolean showStats) { - if (!indexDetailsList.isEmpty()) { - final TabularResultData indexData = ResultBuilder.createTabularResultData(); - - for (final IndexDetails indexDetails : indexDetailsList) { - indexData.accumulate("Member Name", - StringUtils.defaultString(indexDetails.getMemberName())); - indexData.accumulate("Member ID", indexDetails.getMemberId()); - indexData.accumulate("Region Path", indexDetails.getRegionPath()); - indexData.accumulate("Name", indexDetails.getIndexName()); - if (indexDetails.getIndexType() == null) { - indexData.accumulate("Type", ""); - } else { - indexData.accumulate("Type", indexDetails.getIndexType().getName()); - } - indexData.accumulate("Indexed Expression", indexDetails.getIndexedExpression()); - indexData.accumulate("From Clause", indexDetails.getFromClause()); - indexData.accumulate("Valid Index", indexDetails.getIsValid()); - - if (showStats) { - final IndexStatisticsDetailsAdapter adapter = - new IndexStatisticsDetailsAdapter(indexDetails.getIndexStatisticsDetails()); - - indexData.accumulate("Uses", adapter.getTotalUses()); - indexData.accumulate("Updates", adapter.getNumberOfUpdates()); - indexData.accumulate("Update Time", adapter.getTotalUpdateTime()); - indexData.accumulate("Keys", adapter.getNumberOfKeys()); - indexData.accumulate("Values", adapter.getNumberOfValues()); - } + for (final IndexDetails indexDetails : indexDetailsList) { + indexTable.accumulate("Member Name", + StringUtils.defaultString(indexDetails.getMemberName())); + indexTable.accumulate("Member ID", indexDetails.getMemberId()); + indexTable.accumulate("Region Path", indexDetails.getRegionPath()); + indexTable.accumulate("Name", indexDetails.getIndexName()); + if (indexDetails.getIndexType() == null) { + indexTable.accumulate("Type", ""); + } else { + indexTable.accumulate("Type", indexDetails.getIndexType().getName()); + } + indexTable.accumulate("Indexed Expression", indexDetails.getIndexedExpression()); + indexTable.accumulate("From Clause", indexDetails.getFromClause()); + indexTable.accumulate("Valid Index", indexDetails.getIsValid() + ""); + + if (showStats) { + final IndexStatisticsDetailsAdapter adapter = + new IndexStatisticsDetailsAdapter(indexDetails.getIndexStatisticsDetails()); + + indexTable.accumulate("Uses", adapter.getTotalUses()); + indexTable.accumulate("Updates", adapter.getNumberOfUpdates()); + indexTable.accumulate("Update Time", adapter.getTotalUpdateTime()); + indexTable.accumulate("Keys", adapter.getNumberOfKeys()); + indexTable.accumulate("Values", adapter.getNumberOfValues()); } - - return ResultBuilder.buildResult(indexData); - } else { - return ResultBuilder.createInfoResult(CliStrings.LIST_INDEX__INDEXES_NOT_FOUND_MESSAGE); } + + return result; } List getIndexListing() { @@ -122,28 +119,28 @@ public IndexStatisticsDetails getIndexStatisticsDetails() { } public String getNumberOfKeys() { - return getIndexStatisticsDetails() != null - ? StringUtils.defaultString(getIndexStatisticsDetails().getNumberOfKeys()) : ""; + return (getIndexStatisticsDetails() != null) + ? Objects.toString(getIndexStatisticsDetails().getNumberOfKeys(), "") : ""; } public String getNumberOfUpdates() { return getIndexStatisticsDetails() != null - ? StringUtils.defaultString(getIndexStatisticsDetails().getNumberOfUpdates()) : ""; + ? Objects.toString(getIndexStatisticsDetails().getNumberOfUpdates(), "") : ""; } public String getNumberOfValues() { return getIndexStatisticsDetails() != null - ? StringUtils.defaultString(getIndexStatisticsDetails().getNumberOfValues()) : ""; + ? Objects.toString(getIndexStatisticsDetails().getNumberOfValues(), "") : ""; } public String getTotalUpdateTime() { return getIndexStatisticsDetails() != null - ? StringUtils.defaultString(getIndexStatisticsDetails().getTotalUpdateTime()) : ""; + ? Objects.toString(getIndexStatisticsDetails().getTotalUpdateTime(), "") : ""; } public String getTotalUses() { return getIndexStatisticsDetails() != null - ? StringUtils.defaultString(getIndexStatisticsDetails().getTotalUses()) : ""; + ? Objects.toString(getIndexStatisticsDetails().getTotalUses(), "") : ""; } } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXRenameCommand.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXRenameCommand.java index e5024af94583..7bf6e79f3738 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXRenameCommand.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/PDXRenameCommand.java @@ -25,16 +25,16 @@ import org.apache.geode.internal.cache.DiskStoreImpl; import org.apache.geode.management.cli.CliMetaData; -import org.apache.geode.management.cli.Result; +import org.apache.geode.management.cli.GfshCommand; import org.apache.geode.management.internal.cli.i18n.CliStrings; -import org.apache.geode.management.internal.cli.result.ResultBuilder; +import org.apache.geode.management.internal.cli.result.model.ResultModel; import org.apache.geode.pdx.internal.EnumInfo; import org.apache.geode.pdx.internal.PdxType; -public class PDXRenameCommand extends InternalGfshCommand { +public class PDXRenameCommand extends GfshCommand { @CliCommand(value = CliStrings.PDX_RENAME, help = CliStrings.PDX_RENAME__HELP) @CliMetaData(shellOnly = true, relatedTopic = {CliStrings.TOPIC_GEODE_DISKSTORE}) - public Result pdxRename(@CliOption(key = CliStrings.PDX_RENAME_OLD, mandatory = true, + public ResultModel pdxRename(@CliOption(key = CliStrings.PDX_RENAME_OLD, mandatory = true, help = CliStrings.PDX_RENAME_OLD__HELP) String oldClassName, @CliOption(key = CliStrings.PDX_RENAME_NEW, mandatory = true, @@ -44,38 +44,33 @@ public Result pdxRename(@CliOption(key = CliStrings.PDX_RENAME_OLD, mandatory = help = CliStrings.PDX_DISKSTORE__HELP) String diskStore, @CliOption(key = CliStrings.PDX_DISKDIR, mandatory = true, - help = CliStrings.PDX_DISKDIR__HELP) String[] diskDirs) { + help = CliStrings.PDX_DISKDIR__HELP) String[] diskDirs) + throws Exception { - try { - final File[] dirs = new File[diskDirs.length]; - for (int i = 0; i < diskDirs.length; i++) { - dirs[i] = new File((diskDirs[i])); - } - Collection results = - DiskStoreImpl.pdxRename(diskStore, dirs, oldClassName, newClassName); + final File[] dirs = new File[diskDirs.length]; + for (int i = 0; i < diskDirs.length; i++) { + dirs[i] = new File((diskDirs[i])); + } - if (results.isEmpty()) { - return ResultBuilder - .createGemFireErrorResult(CliStrings.format(CliStrings.PDX_RENAME__EMPTY)); - } + Collection results = + DiskStoreImpl.pdxRename(diskStore, dirs, oldClassName, newClassName); - ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); - PrintStream printStream = new PrintStream(outputStream); - for (Object p : results) { - if (p instanceof PdxType) { - ((PdxType) p).toStream(printStream, false); - } else { - ((EnumInfo) p).toStream(printStream); - } - } - String resultString = - CliStrings.format(CliStrings.PDX_RENAME__SUCCESS, outputStream.toString()); - return ResultBuilder.createInfoResult(resultString); + if (results.isEmpty()) { + return ResultModel.createError(CliStrings.format(CliStrings.PDX_RENAME__EMPTY)); + } - } catch (Exception e) { - return ResultBuilder.createGemFireErrorResult( - CliStrings.format(CliStrings.PDX_RENAME__ERROR, e.getMessage())); + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + PrintStream printStream = new PrintStream(outputStream); + for (Object p : results) { + if (p instanceof PdxType) { + ((PdxType) p).toStream(printStream, false); + } else { + ((EnumInfo) p).toStream(printStream); + } } + String resultString = + CliStrings.format(CliStrings.PDX_RENAME__SUCCESS, outputStream.toString()); + return ResultModel.createInfo(resultString); } } From 458e39b8ecaba4ed52f873f33e9c30df60d67c5d Mon Sep 17 00:00:00 2001 From: jinmeiliao Date: Fri, 29 Mar 2019 15:17:26 -0700 Subject: [PATCH 2/8] GEODE-6443: log all request and response in geode management service (#3373) --- .../ManagementRequestLoggingDUnitTest.java | 33 ++++----- .../ClusterManagementRestLoggingTest.java | 45 ++++++++++++ .../cache/configuration/RegionConfigTest.java | 6 -- .../cache/configuration/RegionConfig.java | 2 +- .../ClientClusterManagementService.java | 4 ++ .../rest/ManagementLoggingFilter.java | 70 +++++++++++++++---- 6 files changed, 121 insertions(+), 39 deletions(-) create mode 100644 geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java diff --git a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java index 68f4b6f7d0b3..02d468544e9a 100644 --- a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java +++ b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java @@ -20,7 +20,6 @@ import java.util.List; import java.util.Map; -import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.Appender; import org.apache.logging.log4j.core.LogEvent; @@ -33,45 +32,39 @@ import org.apache.geode.cache.RegionShortcut; import org.apache.geode.cache.configuration.RegionConfig; import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.client.ClusterManagementServiceProvider; import org.apache.geode.test.dunit.rules.ClusterStartupRule; import org.apache.geode.test.dunit.rules.MemberVM; -import org.apache.geode.test.junit.rules.GeodeDevRestClient; public class ManagementRequestLoggingDUnitTest { @ClassRule public static ClusterStartupRule cluster = new ClusterStartupRule(); - private static MemberVM locator, server; - - private static GeodeDevRestClient restClient; + private static ClusterManagementService service; @BeforeClass public static void beforeClass() { locator = cluster.startLocatorVM(0, l -> l.withHttpService()); server = cluster.startServerVM(1, locator.getPort()); - restClient = - new GeodeDevRestClient("/geode-management/v2", "localhost", locator.getHttpPort(), false); + service = ClusterManagementServiceProvider.getService("localhost", locator.getHttpPort()); } @Test public void checkRequestsAreLogged() throws Exception { locator.invoke(() -> { Logger logger = (Logger) LogManager.getLogger(ManagementLoggingFilter.class); - logger.addAppender(new ListAppender("ListAppender")); + ListAppender listAppender = new ListAppender("ListAppender"); + logger.addAppender(listAppender); + listAppender.start(); }); RegionConfig regionConfig = new RegionConfig(); regionConfig.setName("customers"); regionConfig.setType(RegionShortcut.REPLICATE); - ObjectMapper mapper = new ObjectMapper(); - String json = mapper.writeValueAsString(regionConfig); - - ClusterManagementResult result = - restClient.doPostAndAssert("/regions", json) - .hasStatusCode(201) - .getClusterManagementResult(); + ClusterManagementResult result = service.create(regionConfig); assertThat(result.isSuccessful()).isTrue(); locator.invoke(() -> { @@ -80,9 +73,13 @@ public void checkRequestsAreLogged() throws Exception { ListAppender listAppender = (ListAppender) appenders.get("ListAppender"); List logEvents = listAppender.getEvents(); - assertThat(logEvents.size()).as("Expected LogEvents").isEqualTo(1); - assertThat(logEvents.get(0).getMessage().getFormattedMessage()) - .startsWith("Management request:"); + assertThat(logEvents.size()).as("Expected LogEvents").isEqualTo(2); + String beforeMessage = logEvents.get(0).getMessage().getFormattedMessage(); + assertThat(beforeMessage).startsWith("Management Request: POST"); + assertThat(beforeMessage).contains("user=").contains("payload={") + .contains("regionAttributes"); + assertThat(logEvents.get(1).getMessage().getFormattedMessage()) + .startsWith("Management Response: ").contains("response={").contains("Status="); logger.removeAppender(listAppender); }); diff --git a/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java new file mode 100644 index 000000000000..c549e0de4112 --- /dev/null +++ b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java @@ -0,0 +1,45 @@ +/* + * 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.rest.controllers; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.internal.ClientClusterManagementService; +import org.apache.geode.test.junit.rules.LocatorStarterRule; + +public class ClusterManagementRestLoggingTest { + @ClassRule + public static LocatorStarterRule locator = new LocatorStarterRule().withHttpService() + .withSystemProperty("geode.management.request.logging", "false").withAutoStart(); + + private static ClusterManagementService cms; + + @BeforeClass + public static void beforeClass() throws Exception { + cms = new ClientClusterManagementService("localhost", locator.getHttpPort()); + } + + @Test + public void ping() throws Exception { + assertThat(cms.isConnected()).isTrue(); + } + +} diff --git a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java index d78c97fb75fb..fa7e298c2d3d 100644 --- a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java +++ b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java @@ -45,12 +45,6 @@ public void before() throws Exception { service.unMarshall(FileUtils.readFileToString(new File(xmlResource.getFile()), "UTF-8")); } - @Test - public void regionNameCannotBeNull() { - assertThatThrownBy(() -> regionConfig.setName(null)) - .isInstanceOf(IllegalArgumentException.class); - } - @Test public void regionNameSwallowsSlash() { regionConfig.setName("/regionA"); diff --git a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java index 67df3ca8af43..fb6e831bc6b0 100644 --- a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java +++ b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java @@ -324,7 +324,7 @@ public String getName() { */ public void setName(String value) throws IllegalArgumentException { if (value == null) { - throw new IllegalArgumentException("Region name cannot be null"); + return; } boolean regionPrefixedWithSlash = value.startsWith("/"); diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java index baa5db3e71ce..f3d7cff52055 100644 --- a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java +++ b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java @@ -65,6 +65,10 @@ private ClientClusterManagementService() { restTemplate.setErrorHandler(DEFAULT_ERROR_HANDLER); } + public ClientClusterManagementService(String host, int port) { + this(host, port, null, null, null, null); + } + public ClientClusterManagementService(String host, int port, SSLContext sslContext, HostnameVerifier hostnameVerifier, String username, String password) { this(); diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java index d827266cabe2..90bd55678e4c 100644 --- a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java +++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java @@ -15,32 +15,74 @@ package org.apache.geode.management.internal.rest; +import java.io.IOException; +import java.io.UnsupportedEncodingException; + +import javax.servlet.FilterChain; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; -import org.springframework.web.filter.AbstractRequestLoggingFilter; +import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.util.ContentCachingRequestWrapper; +import org.springframework.web.util.ContentCachingResponseWrapper; -public class ManagementLoggingFilter extends AbstractRequestLoggingFilter { +public class ManagementLoggingFilter extends OncePerRequestFilter { // Because someone is going to want to disable this. private static final Boolean ENABLE_REQUEST_LOGGING = Boolean.parseBoolean(System.getProperty("geode.management.request.logging", "true")); - public ManagementLoggingFilter() { - super.setIncludeQueryString(true); - super.setIncludePayload(true); - super.setMaxPayloadLength(1000); - super.setAfterMessagePrefix("Management request: ["); - } + private static int MAX_PAYLOAD_LENGTH = 10000; @Override - protected void beforeRequest(HttpServletRequest request, String message) { - // No logging here - this would not display the payload + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + + if (!ENABLE_REQUEST_LOGGING) { + filterChain.doFilter(request, response); + return; + } + + // We can not log request payload before making the actual request because then the InputStream + // would be consumed and cannot be read again by the actual processing/server. + ContentCachingRequestWrapper wrappedRequest = new ContentCachingRequestWrapper(request); + ContentCachingResponseWrapper wrappedResponse = new ContentCachingResponseWrapper(response); + + // performs the actual request before logging + filterChain.doFilter(wrappedRequest, wrappedResponse); + + // log after the request has been made and ContentCachingRequestWrapper has cached the request + // payload. + String requestPattern = "Management Request: %s[url=%s]; user=%s; payload=%s"; + String requestUrl = request.getRequestURI(); + if (request.getQueryString() != null) { + requestUrl = requestUrl + "?" + request.getQueryString(); + } + String payload = getContentAsString(wrappedRequest.getContentAsByteArray(), + wrappedRequest.getCharacterEncoding()); + logger.info(String.format(requestPattern, request.getMethod(), requestUrl, + request.getRemoteUser(), payload)); + + // construct the response message + String responsePattern = "Management Response: Status=%s; response=%s"; + payload = getContentAsString(wrappedResponse.getContentAsByteArray(), + wrappedResponse.getCharacterEncoding()); + payload = payload.replaceAll(System.lineSeparator(), ""); + logger.info(String.format(responsePattern, response.getStatus(), payload)); + + // IMPORTANT: copy content of response back into original response + wrappedResponse.copyBodyToResponse(); } - @Override - protected void afterRequest(HttpServletRequest request, String message) { - if (ENABLE_REQUEST_LOGGING) { - logger.info(message); + private String getContentAsString(byte[] buf, String encoding) { + if (buf == null || buf.length == 0) + return ""; + int length = Math.min(buf.length, MAX_PAYLOAD_LENGTH); + try { + return new String(buf, 0, length, encoding); + } catch (UnsupportedEncodingException ex) { + return "[unknown]"; } } } From fbbefd399aa287cec4f9e096eaa32c0562537e1d Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Mon, 1 Apr 2019 09:37:34 -0700 Subject: [PATCH 3/8] GEODE-6578: Moves MockExtension out of geode-junit. (#3377) It doesn't need to be in this shared test framework. --- .../internal/cache/extension/mock/AbstractMockExtension.java | 0 .../extension/mock/AbstractMockExtensionXmlGenerator.java | 0 .../extension/mock/AlterMockCacheExtensionFunction.java | 0 .../extension/mock/AlterMockRegionExtensionFunction.java | 0 .../extension/mock/CreateMockCacheExtensionFunction.java | 0 .../extension/mock/CreateMockRegionExtensionFunction.java | 0 .../extension/mock/DestroyMockCacheExtensionFunction.java | 0 .../extension/mock/DestroyMockRegionExtensionFunction.java | 0 .../internal/cache/extension/mock/MockCacheExtension.java | 0 .../cache/extension/mock/MockCacheExtensionXmlGenerator.java | 0 .../internal/cache/extension/mock/MockExtensionCommands.java | 0 .../cache/extension/mock/MockExtensionXmlParser.java | 0 .../internal/cache/extension/mock/MockRegionExtension.java | 0 .../extension/mock/MockRegionExtensionXmlGenerator.java | 0 .../org.apache.geode.internal.cache.xmlcache.XmlParser | 0 .../services/org.springframework.shell.core.CommandMarker | 0 .../cli/commands/CommandAvailabilityIndicatorTestHelper.java | 5 ----- 17 files changed, 5 deletions(-) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtension.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtensionXmlGenerator.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/AlterMockCacheExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/AlterMockRegionExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/CreateMockCacheExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/CreateMockRegionExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/DestroyMockCacheExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/DestroyMockRegionExtensionFunction.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtension.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtensionXmlGenerator.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockExtensionXmlParser.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtension.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtensionXmlGenerator.java (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/resources/META-INF/services/org.apache.geode.internal.cache.xmlcache.XmlParser (100%) rename {geode-junit/src/main => geode-core/src/distributedTest}/resources/META-INF/services/org.springframework.shell.core.CommandMarker (100%) diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtension.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtension.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtension.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtension.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtensionXmlGenerator.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtensionXmlGenerator.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtensionXmlGenerator.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AbstractMockExtensionXmlGenerator.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AlterMockCacheExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AlterMockCacheExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AlterMockCacheExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AlterMockCacheExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AlterMockRegionExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AlterMockRegionExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/AlterMockRegionExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/AlterMockRegionExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/CreateMockCacheExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/CreateMockCacheExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/CreateMockCacheExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/CreateMockCacheExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/CreateMockRegionExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/CreateMockRegionExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/CreateMockRegionExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/CreateMockRegionExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/DestroyMockCacheExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/DestroyMockCacheExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/DestroyMockCacheExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/DestroyMockCacheExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/DestroyMockRegionExtensionFunction.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/DestroyMockRegionExtensionFunction.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/DestroyMockRegionExtensionFunction.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/DestroyMockRegionExtensionFunction.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtension.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtension.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtension.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtension.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtensionXmlGenerator.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtensionXmlGenerator.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtensionXmlGenerator.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockCacheExtensionXmlGenerator.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionCommands.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockExtensionXmlParser.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionXmlParser.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockExtensionXmlParser.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockExtensionXmlParser.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtension.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtension.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtension.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtension.java diff --git a/geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtensionXmlGenerator.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtensionXmlGenerator.java similarity index 100% rename from geode-junit/src/main/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtensionXmlGenerator.java rename to geode-core/src/distributedTest/java/org/apache/geode/internal/cache/extension/mock/MockRegionExtensionXmlGenerator.java diff --git a/geode-junit/src/main/resources/META-INF/services/org.apache.geode.internal.cache.xmlcache.XmlParser b/geode-core/src/distributedTest/resources/META-INF/services/org.apache.geode.internal.cache.xmlcache.XmlParser similarity index 100% rename from geode-junit/src/main/resources/META-INF/services/org.apache.geode.internal.cache.xmlcache.XmlParser rename to geode-core/src/distributedTest/resources/META-INF/services/org.apache.geode.internal.cache.xmlcache.XmlParser diff --git a/geode-junit/src/main/resources/META-INF/services/org.springframework.shell.core.CommandMarker b/geode-core/src/distributedTest/resources/META-INF/services/org.springframework.shell.core.CommandMarker similarity index 100% rename from geode-junit/src/main/resources/META-INF/services/org.springframework.shell.core.CommandMarker rename to geode-core/src/distributedTest/resources/META-INF/services/org.springframework.shell.core.CommandMarker diff --git a/geode-junit/src/main/java/org/apache/geode/management/internal/cli/commands/CommandAvailabilityIndicatorTestHelper.java b/geode-junit/src/main/java/org/apache/geode/management/internal/cli/commands/CommandAvailabilityIndicatorTestHelper.java index 94c9821bd8c4..f596b45b3885 100644 --- a/geode-junit/src/main/java/org/apache/geode/management/internal/cli/commands/CommandAvailabilityIndicatorTestHelper.java +++ b/geode-junit/src/main/java/org/apache/geode/management/internal/cli/commands/CommandAvailabilityIndicatorTestHelper.java @@ -23,7 +23,6 @@ import org.springframework.shell.core.CommandMarker; import org.springframework.shell.core.annotation.CliCommand; -import org.apache.geode.internal.cache.extension.mock.MockExtensionCommands; import org.apache.geode.management.cli.CliMetaData; import org.apache.geode.management.cli.GfshCommand; import org.apache.geode.management.internal.cli.CommandManager; @@ -39,10 +38,6 @@ public static void assertOnlineCommandsHasAvailabilityIndicator(CommandManager m continue; } - if (commandMarker instanceof MockExtensionCommands) { - continue; - } - for (Method method : commandMarker.getClass().getMethods()) { CliCommand cliCommand = method.getAnnotation(CliCommand.class); if (cliCommand == null) { From e87e720b331f164723c36ec82cd8d01c559b1ae3 Mon Sep 17 00:00:00 2001 From: Helena Bales Date: Mon, 1 Apr 2019 10:00:49 -0700 Subject: [PATCH 4/8] GEODE-6515: refactor ConnectionManagerImpl (#3304) Refactors the ConnectionManagerImpl to a non-locking implementation to allow gets to scale with more threads. The previous implementation locked around all logic for getting, creating, or returning a connection to the pool, which resulted in a high degree of contention for that lock. Additionally, much of the logic for accounting for the number of total connections, and the dequeue of available connections have been extracted to ConnectionAccounting and AvailableConnectionManager respectively. This was done in order to add unit and concurrent tests for that logic. * Refactor ConnectionManagerImpl to a non-locking implementation * add unit tests for ConnectionManagerImpl * update ConnectionManagerImpl Javadocs * extract ConnectionAccounting from ConnectionManagerImpl * add unit test for ConnectionAccounting * add concurrency tests for ConnectionAccounting * extract AvailableConnectionManager from ConnectionManagerImpl * add unit tests for AvailableConnectionManager * add concurrency tests for AvailableConnectionManager * add unit test for ConcurrentTestRunner * add javadocs to AvailableConnectionManager and improved the method names * activate returns false if the connection has been destroyed instead of throwing ConnectionDestroyedException * start background prefill if under the minimum number of connections in ConnectionManagerImpl#borrowConnection when create fails * add generic to Set in ConnectionManagerImpl * Correct invalidateServer logic in ConnectionManagerImpl * make NOT_WAITING private in ConnectionManagerImpl * made createLifetimeReplacementConnection private since it is only used by ConnectionMap Signed-off-by: Helena Bales Signed-off-by: Jacob Barrett Signed-off-by: Darrel Schneider --- geode-concurrency-test/build.gradle | 8 +- .../concurrency/ConcurrentTestRunner.java | 4 +- .../test/concurrency/ParallelExecutor.java | 11 + .../geode/test/concurrency/Utilities.java | 38 + .../test/concurrency/loop/LoopRunner.java | 28 +- .../concurrency/loop/LoopRunnerConfig.java | 7 + .../concurrency/ConcurrentTestRunnerTest.java | 58 ++ ...ilableConnectionManagerConcurrentTest.java | 189 ++++ .../ConnectionAccountingConcurrentTest.java | 211 +++++ .../pooling/ConnectionManagerImplTest.java | 559 +++++++++++ .../pooling/ConnectionManagerJUnitTest.java | 2 +- .../cache/client/internal/OpExecutorImpl.java | 12 +- .../pooling/AvailableConnectionManager.java | 143 +++ .../pooling/ConnectionAccounting.java | 148 +++ .../internal/pooling/ConnectionManager.java | 42 +- .../pooling/ConnectionManagerImpl.java | 864 +++++++----------- .../internal/pooling/PooledConnection.java | 21 +- .../internal/OpExecutorImplJUnitTest.java | 7 +- .../AvailableConnectionManagerTest.java | 230 +++++ .../pooling/ConnectionAccountingTest.java | 223 +++++ 20 files changed, 2244 insertions(+), 561 deletions(-) create mode 100644 geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/Utilities.java create mode 100644 geode-concurrency-test/src/test/java/org/apache/geode/test/concurrency/ConcurrentTestRunnerTest.java create mode 100644 geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerConcurrentTest.java create mode 100644 geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingConcurrentTest.java create mode 100644 geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImplTest.java create mode 100644 geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java create mode 100644 geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccounting.java create mode 100644 geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java create mode 100644 geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingTest.java diff --git a/geode-concurrency-test/build.gradle b/geode-concurrency-test/build.gradle index 7d422c82e81e..3ab10bb5c19b 100644 --- a/geode-concurrency-test/build.gradle +++ b/geode-concurrency-test/build.gradle @@ -14,11 +14,17 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -apply from: "${project.projectDir}/../gradle/publish.gradle" +apply from: "${project.projectDir}/../gradle/publish.gradle" dependencies { compile(platform(project(':boms:geode-all-bom'))) compile('junit:junit') compile('org.apache.logging.log4j:log4j-api') + testCompile('org.assertj:assertj-core') +} + +test { + // Some tests have inner tests that should be ignored + exclude "**/*\$*.class" } diff --git a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ConcurrentTestRunner.java b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ConcurrentTestRunner.java index 177bc551526e..5652e1e6f4b0 100644 --- a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ConcurrentTestRunner.java +++ b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ConcurrentTestRunner.java @@ -38,9 +38,7 @@ * test can use to invoke code in parallel. * * This test run will try to exercise the test method to flush out any concurrent bugs in the - * parallel execution. Currently this runner is using Java PathFinder to run the test with *all* - * possible thread interleavings, but other methods such as invoking the method multiple times in a - * normal JVM may be supported in the feature. + * parallel execution. * * All test logic and state *must* be encapsulated in the individual test methods. This is because * the concurrency testing logic may need to invoke the test body multiple times, possibly in diff --git a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ParallelExecutor.java b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ParallelExecutor.java index ee46ed8f533d..011c123b7f59 100644 --- a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ParallelExecutor.java +++ b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/ParallelExecutor.java @@ -14,6 +14,8 @@ */ package org.apache.geode.test.concurrency; +import java.util.ArrayList; +import java.util.Collection; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; @@ -40,9 +42,18 @@ default Future inParallel(RunnableWithException runnable) { }); } + default Collection> inParallel(RunnableWithException runnable, int count) { + ArrayList> futures = new ArrayList<>(count); + for (; count > 0; count--) { + futures.add(inParallel(runnable)); + } + return futures; + } + /** * Execute all tasks in parallel, wait for them to complete and throw an exception if any of the * tasks throw an exception. */ void execute() throws ExecutionException, InterruptedException; + } diff --git a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/Utilities.java b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/Utilities.java new file mode 100644 index 000000000000..bc274f7e91f2 --- /dev/null +++ b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/Utilities.java @@ -0,0 +1,38 @@ +/* + * 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.test.concurrency; + +public interface Utilities { + + /** + * @return Result of {@code Runtime.getRuntime().availableProcessors()}. + */ + static int availableProcessors() { + return Runtime.getRuntime().availableProcessors(); + } + + /** + * Repeat {@code task} for {@code count} times serially. + * + * @param task to repeat + * @param count number of times + */ + static void repeat(Runnable task, int count) { + for (; count > 0; count--) { + task.run(); + } + } +} diff --git a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunner.java b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunner.java index 31d87f9f041a..0cc029765f7f 100644 --- a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunner.java +++ b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunner.java @@ -12,17 +12,21 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ + package org.apache.geode.test.concurrency.loop; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.geode.test.concurrency.ParallelExecutor; import org.apache.geode.test.concurrency.Runner; @@ -44,6 +48,12 @@ public List runTestMethod(Method child) { try { Object test = child.getDeclaringClass().newInstance(); child.invoke(test, executor); + } catch (InvocationTargetException ex) { + Throwable exceptionToReturn = ex.getCause(); + if (exceptionToReturn == null) { + exceptionToReturn = ex; + } + return Collections.singletonList(ex.getCause()); } catch (Exception e) { return Collections.singletonList(e); } @@ -66,25 +76,37 @@ private int getCount(Method child) { private static class DelegatingExecutor implements ParallelExecutor { private final ExecutorService executorService; - List> futures; + private List> futures; + private final AtomicInteger callablesStarting = new AtomicInteger(0); + private final CountDownLatch start = new CountDownLatch(1); public DelegatingExecutor(ExecutorService executorService) { this.executorService = executorService; - futures = new ArrayList>(); + futures = new ArrayList<>(); } @Override public Future inParallel(Callable callable) { - Future future = executorService.submit(callable); + callablesStarting.getAndIncrement(); + Future future = executorService.submit(() -> { + callablesStarting.getAndDecrement(); + start.await(); + return callable.call(); + }); futures.add(future); return future; } @Override public void execute() throws ExecutionException, InterruptedException { + while (callablesStarting.get() > 0); + + start.countDown(); for (Future future : futures) { future.get(); } + futures.clear(); } + } } diff --git a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunnerConfig.java b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunnerConfig.java index 7e58a4e7d726..ee294d72f83d 100644 --- a/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunnerConfig.java +++ b/geode-concurrency-test/src/main/java/org/apache/geode/test/concurrency/loop/LoopRunnerConfig.java @@ -14,9 +14,16 @@ */ package org.apache.geode.test.concurrency.loop; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + /** * Configuration for the LoopRunner class */ +@Retention(RetentionPolicy.RUNTIME) +@Target(ElementType.TYPE) public @interface LoopRunnerConfig { int count(); } diff --git a/geode-concurrency-test/src/test/java/org/apache/geode/test/concurrency/ConcurrentTestRunnerTest.java b/geode-concurrency-test/src/test/java/org/apache/geode/test/concurrency/ConcurrentTestRunnerTest.java new file mode 100644 index 000000000000..76e8d0124f80 --- /dev/null +++ b/geode-concurrency-test/src/test/java/org/apache/geode/test/concurrency/ConcurrentTestRunnerTest.java @@ -0,0 +1,58 @@ +/* + * 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.test.concurrency; + +import static org.apache.geode.test.concurrency.Utilities.availableProcessors; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.ExecutionException; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.Test; +import org.junit.runner.JUnitCore; +import org.junit.runner.RunWith; + +public class ConcurrentTestRunnerTest { + @Test + public void confirmThatInParallelRunsConcurrently() { + // We only need FailingTest to fail once for the following + // assertion to pass. ConcurrentTestRunner runs FailingTest + // 1000 times by default. It will stop running it once it + // sees it fail, which is what we want to see because it + // confirms that running inParallel actually runs concurrently. + assertThat(JUnitCore.runClasses(CheckForConcurrency.class).wasSuccessful()).isFalse(); + } + + /** + * This "test" is only meant to be run by confirmThatInParallelRunsConcurrently. + * If you run this "test" directly you can expect to see if fail. + */ + @RunWith(ConcurrentTestRunner.class) + public static class CheckForConcurrency { + @Test + public void validateConcurrentExecution(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + final AtomicInteger atomicInteger = new AtomicInteger(0); + executor.inParallel(() -> { + int oldValue = atomicInteger.get(); + // We want to see the following assertion fail because that indicates + // that another thread currently modified the atomic. + assertThat(atomicInteger.compareAndSet(oldValue, oldValue + 1)).isTrue(); + }, availableProcessors()); + executor.execute(); + } + } +} diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerConcurrentTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerConcurrentTest.java new file mode 100644 index 000000000000..de86b9629274 --- /dev/null +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerConcurrentTest.java @@ -0,0 +1,189 @@ +/* + * 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.cache.client.internal.pooling; + +import static org.apache.geode.test.concurrency.Utilities.availableProcessors; +import static org.apache.geode.test.concurrency.Utilities.repeat; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.concurrent.ExecutionException; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.test.concurrency.ConcurrentTestRunner; +import org.apache.geode.test.concurrency.ParallelExecutor; + +@RunWith(ConcurrentTestRunner.class) +public class AvailableConnectionManagerConcurrentTest { + private final int parallelCount = availableProcessors(); + private final int iterationCount = 250; + private final AvailableConnectionManager instance = new AvailableConnectionManager(); + + @Test + public void useFirstAddFirstDoesNotLoseConnections(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + repeat(() -> instance.addFirst(createConnection(), false), parallelCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(); + instance.addFirst(used, true); + }, iterationCount); + }, parallelCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(parallelCount); + } + + @Test + public void useFirstWithPredicateAddFirstDoesNotLoseConnections(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + repeat(() -> instance.addFirst(createConnection(), false), parallelCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(c -> true); + instance.addFirst(used, true); + }, iterationCount); + }, parallelCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(parallelCount); + } + + @Test + public void useFirstAddLastDoesNotLoseConnections(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + repeat(() -> instance.addFirst(createConnection(), false), parallelCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(); + instance.addLast(used, true); + }, iterationCount); + }, parallelCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(parallelCount); + } + + @Test + public void useFirstAddFirstDoesNotLoseConnectionsEvenWhenUseFirstReturnsNull( + ParallelExecutor executor) + throws ExecutionException, InterruptedException { + int connectionCount = 2; + int threadCount = connectionCount * 5; + repeat(() -> instance.addFirst(createConnection(), false), connectionCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(); + if (used != null) { + Thread.yield(); + instance.addFirst(used, true); + } + }, iterationCount); + }, threadCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(connectionCount); + } + + @Test + public void useFirstWithPredicateAddFirstDoesNotLoseConnectionsEvenWhenUseFirstReturnsNull( + ParallelExecutor executor) + throws ExecutionException, InterruptedException { + int connectionCount = 2; + int threadCount = connectionCount * 5; + repeat(() -> instance.addFirst(createConnection(), false), connectionCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(c -> true); + if (used != null) { + Thread.yield(); + instance.addFirst(used, true); + } + }, iterationCount); + }, threadCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(connectionCount); + } + + @Test + public void useFirstAddLastWithPredicateThatDoesNotAlwaysMatchDoesNotLoseConnectionsEvenWhenUseFirstReturnsNull( + ParallelExecutor executor) + throws ExecutionException, InterruptedException { + int connectionCount = 2; + int threadCount = connectionCount * 5; + repeat(() -> instance.addFirst(createConnection(), false), connectionCount); + // now add a bunch of connections that will not match the predicate + repeat(() -> { + PooledConnection nonMatchingConnection = createConnection(); + when(nonMatchingConnection.getBirthDate()).thenReturn(1L); + instance.addFirst(nonMatchingConnection, false); + }, connectionCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection used = instance.useFirst(c -> c.getBirthDate() == 0L); + if (used != null) { + Thread.yield(); + assertThat(used.getBirthDate()).isEqualTo(0L); + instance.addLast(used, true); + } + }, iterationCount); + }, threadCount); + executor.execute(); + + assertThat(instance.getDeque()).hasSize(connectionCount * 2); + } + + @Test + public void addLastRemoveDoesNotRemoveOtherConnections(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + int originalCount = 7; + ArrayList originalConnections = new ArrayList<>(); + repeat(() -> { + PooledConnection original = createConnection(); + originalConnections.add(original); + instance.addFirst(original, false); + }, originalCount); + + executor.inParallel(() -> { + repeat(() -> { + PooledConnection removed = createConnection(); + instance.addLast(removed, true); + assertThat(instance.remove(removed)).isTrue(); + }, iterationCount); + }, parallelCount); + executor.execute(); + + assertThat(instance.getDeque()).containsExactlyInAnyOrderElementsOf(originalConnections); + } + + private PooledConnection createConnection() { + PooledConnection result = mock(PooledConnection.class); + when(result.activate()).thenReturn(true); + when(result.isActive()).thenReturn(true); + return result; + } +} diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingConcurrentTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingConcurrentTest.java new file mode 100644 index 000000000000..75ead32082dd --- /dev/null +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingConcurrentTest.java @@ -0,0 +1,211 @@ +/* + * 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.cache.client.internal.pooling; + +import static org.apache.geode.test.concurrency.Utilities.availableProcessors; +import static org.apache.geode.test.concurrency.Utilities.repeat; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.concurrent.ExecutionException; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.test.concurrency.ConcurrentTestRunner; +import org.apache.geode.test.concurrency.ParallelExecutor; + +@RunWith(ConcurrentTestRunner.class) +public class ConnectionAccountingConcurrentTest { + private final int count = availableProcessors() * 2; + + @Test + public void tryPrefillStaysBelowOrAtMin(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + final int min = count - 1; + ConnectionAccounting accountant = new ConnectionAccounting(min, min + 4); + + executor.inParallel(() -> { + accountant.tryPrefill(); + assertThat(accountant.getCount()).isGreaterThan(0).isLessThanOrEqualTo(min); + }, count); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(min); + } + + @Test + public void cancelTryPrefillStaysUnderMin(ParallelExecutor executor) + throws ExecutionException, InterruptedException { + final int min = count - 1; + ConnectionAccounting accountant = new ConnectionAccounting(min, min + 4); + + executor.inParallel(() -> { + if (accountant.tryPrefill()) { + accountant.cancelTryPrefill(); + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(0).isLessThanOrEqualTo(min); + } + }, count); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(0); + } + + @Test + public void creates(ParallelExecutor executor) throws Exception { + ConnectionAccounting accountant = new ConnectionAccounting(0, 1); + + executor.inParallel(() -> { + accountant.create(); + assertThat(accountant.getCount()).isGreaterThan(0).isLessThanOrEqualTo(count); + }, count); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(count); + } + + @Test + public void tryCreateStaysWithinMax(ParallelExecutor executor) throws Exception { + ConnectionAccounting accountant = new ConnectionAccounting(1, count); + + executor.inParallel(() -> { + if (accountant.tryCreate()) { + assertThat(accountant.getCount()).isGreaterThan(0).isLessThanOrEqualTo(count); + } + }, count + 1); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(count); + } + + @Test + public void cancelTryCreateStaysWithinMax(ParallelExecutor executor) throws Exception { + ConnectionAccounting accountant = new ConnectionAccounting(1, count); + + executor.inParallel(() -> { + if (accountant.tryCreate()) { + accountant.cancelTryCreate(); + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(0).isLessThanOrEqualTo(count); + } + }, count + 1); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(0); + } + + @Test + public void destroyAndIsUnderMinimum(ParallelExecutor executor) throws Exception { + ConnectionAccounting accountant = new ConnectionAccounting(2, 4); + repeat(() -> accountant.create(), count); + + executor.inParallel(() -> { + if (accountant.destroyAndIsUnderMinimum(1)) { + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(0).isLessThan(2); + } else { + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(0).isLessThan(count); + } + }, count); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(0); + } + + @Test + public void tryDestroyNeverGoesBelowMax(ParallelExecutor executor) throws Exception { + final int overfillMax = Math.max(count, 4); + final int max = overfillMax / 2; + ConnectionAccounting accountant = new ConnectionAccounting(1, max); + repeat(() -> accountant.create(), overfillMax); + + executor.inParallel(() -> { + if (accountant.tryDestroy()) { + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(max).isLessThan(overfillMax); + } + }, overfillMax + 1); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(max); + } + + @Test + public void cancelTryDestroyStaysAboveMax(ParallelExecutor executor) throws Exception { + final int overfillMax = Math.max(count, 4); + final int max = overfillMax / 2; + ConnectionAccounting accountant = new ConnectionAccounting(1, max); + repeat(() -> accountant.create(), overfillMax); + + executor.inParallel(() -> { + if (accountant.tryDestroy()) { + accountant.cancelTryDestroy(); + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(max) + .isLessThanOrEqualTo(overfillMax); + } + }, max + 1); + + executor.execute(); + + assertThat(accountant.getCount()).isEqualTo(overfillMax); + } + + + @Test + public void mixItUp(ParallelExecutor executor) throws Exception { + final int overfill = Math.max(count, 8); + final int max = overfill / 4; + final int overfillMax = overfill + max; + ConnectionAccounting accountant = new ConnectionAccounting(1, max); + repeat(() -> accountant.create(), overfill); + + executor.inParallel(() -> { + if (accountant.tryDestroy()) { + accountant.cancelTryDestroy(); + } + }, max); + + executor.inParallel(() -> { + if (accountant.tryCreate()) { + accountant.cancelTryCreate(); + } + }, max); + + executor.inParallel(() -> { + accountant.create(); + }, max); + + executor.inParallel(() -> { + if (accountant.tryCreate()) { + accountant.destroyAndIsUnderMinimum(1); + } + }, max); + + executor.inParallel(() -> { + if (accountant.tryPrefill()) { + accountant.cancelTryPrefill(); + } + }, max); + + executor.execute(); + + assertThat(accountant.getCount()).isGreaterThanOrEqualTo(0).isLessThanOrEqualTo(overfillMax); + } + +} diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImplTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImplTest.java new file mode 100644 index 000000000000..35afea368fdc --- /dev/null +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImplTest.java @@ -0,0 +1,559 @@ +/* + * 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.cache.client.internal.pooling; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +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.doNothing; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.ScheduledExecutorService; + +import org.junit.Test; + +import org.apache.geode.CancelCriterion; +import org.apache.geode.InternalGemFireException; +import org.apache.geode.cache.client.AllConnectionsInUseException; +import org.apache.geode.cache.client.NoAvailableServersException; +import org.apache.geode.cache.client.internal.Connection; +import org.apache.geode.cache.client.internal.ConnectionFactory; +import org.apache.geode.cache.client.internal.Endpoint; +import org.apache.geode.cache.client.internal.EndpointManager; +import org.apache.geode.distributed.PoolCancelledException; +import org.apache.geode.distributed.internal.ServerLocation; +import org.apache.geode.internal.cache.PoolStats; +import org.apache.geode.internal.logging.InternalLogWriter; + +public class ConnectionManagerImplTest { + ConnectionManagerImpl connectionManager; + public final String poolName = "poolName"; + public final ConnectionFactory connectionFactory = mock(ConnectionFactory.class); + public final EndpointManager endpointManager = mock(EndpointManager.class); + public final InternalLogWriter securityLogger = mock(InternalLogWriter.class); + public final CancelCriterion cancelCriterion = mock(CancelCriterion.class); + public final PoolStats poolStats = mock(PoolStats.class); + public final ScheduledExecutorService backgroundProcessor = mock(ScheduledExecutorService.class); + public int maxConnections = 800; + public int minConnections = 10; + public long idleTimeout = 1000; + public long timeout = 1000; + public int lifetimeTimeout = 1000; + public long pingInterval = 10; + + private ConnectionManagerImpl createDefaultConnectionManager() { + return new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, maxConnections, + minConnections, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, cancelCriterion, + poolStats); + } + + @Test + public void startExecutedPrefillConnectionsOnce() { + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + connectionManager.start(backgroundProcessor); + verify(backgroundProcessor, times(1)).execute(any()); + + connectionManager.close(false); + } + + @Test + public void startShouldEatRejectedExecutionException() { + doThrow(RejectedExecutionException.class).when(backgroundProcessor).execute(any()); + + connectionManager = createDefaultConnectionManager(); + assertThatCode(() -> connectionManager.start(backgroundProcessor)).doesNotThrowAnyException(); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionThrowsWhenUsingExistingConnectionsAndNoConnectionsExist() { + ServerLocation serverLocation = mock(ServerLocation.class); + + connectionManager = createDefaultConnectionManager(); + assertThatThrownBy(() -> connectionManager.borrowConnection(serverLocation, timeout, true)) + .isInstanceOf(AllConnectionsInUseException.class); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionCreatesAConnectionOnSpecifiedServerWhenNoneExist() { + Connection connection = mock(Connection.class); + ServerLocation serverLocation = mock(ServerLocation.class); + when(connectionFactory.createClientToServerConnection(serverLocation, false)) + .thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + assertThat(connectionManager.borrowConnection(serverLocation, timeout, false)) + .isInstanceOf(PooledConnection.class); + assertThat(connectionManager.getConnectionCount()).isEqualTo(1); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionCreatesAConnectionWhenNoneExist() { + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + assertThat(connectionManager.borrowConnection(timeout)).isInstanceOf(PooledConnection.class); + assertThat(connectionManager.getConnectionCount()).isEqualTo(1); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionReturnsAnActiveConnection() { + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + PooledConnection heldConnection = + (PooledConnection) connectionManager.borrowConnection(timeout); + assertThatThrownBy(() -> heldConnection.activate()).isInstanceOf(InternalGemFireException.class) + .hasMessageContaining("Connection already active"); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionReturnsAConnectionWhenOneExists() { + ServerLocation serverLocation = mock(ServerLocation.class); + Endpoint endpoint = mock(Endpoint.class); + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + when(connection.getServer()).thenReturn(serverLocation); + when(connection.getEndpoint()).thenReturn(endpoint); + when(endpoint.getLocation()).thenReturn(serverLocation); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(timeout); + connectionManager.returnConnection(heldConnection); + heldConnection = connectionManager.borrowConnection(timeout); + assertThat(heldConnection.getServer()).isEqualTo(connection.getServer()); + assertThat(connectionManager.getConnectionCount()).isEqualTo(1); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionThrowsExceptionWhenUnableToCreateConnection() { + when(connectionFactory.createClientToServerConnection(any())).thenReturn(null); + doNothing().when(backgroundProcessor).execute(any()); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + assertThatThrownBy(() -> connectionManager.borrowConnection(timeout)) + .isInstanceOf(NoAvailableServersException.class); + assertThat(connectionManager.getConnectionCount()).isEqualTo(0); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionWillSchedulePrefillIfUnderMinimumConnections() { + when(connectionFactory.createClientToServerConnection(any())).thenReturn(null); + doNothing().when(backgroundProcessor).execute(any()); + + pingInterval = 20000000; // set it high to prevent prefill retry + connectionManager = spy(createDefaultConnectionManager()); + connectionManager.start(backgroundProcessor); + assertThatThrownBy(() -> connectionManager.borrowConnection(timeout)) + .isInstanceOf(NoAvailableServersException.class); + assertThat(connectionManager.getConnectionCount()).isEqualTo(0); + + verify(connectionManager, times(2)).startBackgroundPrefill(); + connectionManager.close(false); + } + + @Test + public void borrowConnectionGivesUpWhenShuttingDown() { + int maxConnections = 1; + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + + connectionManager = new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, + maxConnections, 1, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, + cancelCriterion, poolStats); + connectionManager.start(backgroundProcessor); + connectionManager.shuttingDown.set(true); + + // reach max connection count so we can't create a new connection and end up in the wait loop + connectionManager.borrowConnection(timeout); + assertThat(connectionManager.getConnectionCount()).isEqualTo(maxConnections); + + assertThatThrownBy(() -> connectionManager.borrowConnection(timeout)) + .isInstanceOf(PoolCancelledException.class); + + connectionManager.close(false); + } + + @Test + public void borrowConnectionTimesOutWithException() { + int maxConnections = 1; + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + + connectionManager = new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, + maxConnections, 1, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, + cancelCriterion, poolStats); + connectionManager.start(backgroundProcessor); + + // reach max connection count so we can't create a new connection and end up in the wait loop + connectionManager.borrowConnection(timeout); + assertThat(connectionManager.getConnectionCount()).isEqualTo(maxConnections); + + assertThatThrownBy(() -> connectionManager.borrowConnection(10)) + .isInstanceOf(AllConnectionsInUseException.class); + + connectionManager.close(false); + } + + @Test + public void borrowWithServerLocationBreaksMaxConnectionContract() { + int maxConnections = 2; + + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Connection connection2 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation2, false)) + .thenReturn(connection2); + + ServerLocation serverLocation3 = mock(ServerLocation.class); + Connection connection3 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation3, false)) + .thenReturn(connection3); + + connectionManager = new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, + maxConnections, 1, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, + cancelCriterion, poolStats); + connectionManager.start(backgroundProcessor); + + connectionManager.borrowConnection(serverLocation1, timeout, false); + connectionManager.borrowConnection(serverLocation2, timeout, false); + connectionManager.borrowConnection(serverLocation3, timeout, false); + + assertThat(connectionManager.getConnectionCount()).isGreaterThan(maxConnections); + + connectionManager.close(false); + } + + @Test + public void returnConnectionReturnsToHead() { + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + when(connection1.getServer()).thenReturn(serverLocation1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Connection connection2 = mock(Connection.class); + Endpoint endpoint2 = mock(Endpoint.class); + when(connectionFactory.createClientToServerConnection(serverLocation2, false)) + .thenReturn(connection2); + when(connection2.getServer()).thenReturn(serverLocation2); + when(connection2.getEndpoint()).thenReturn(endpoint2); + when(endpoint2.getLocation()).thenReturn(serverLocation2); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + Connection heldConnection1 = + connectionManager.borrowConnection(serverLocation1, timeout, false); + Connection heldConnection2 = + connectionManager.borrowConnection(serverLocation2, timeout, false); + assertThat(connectionManager.getConnectionCount()).isEqualTo(2); + + connectionManager.returnConnection(heldConnection1, true); + connectionManager.returnConnection(heldConnection2, true); + + assertThat(connectionManager.borrowConnection(timeout).getServer()) + .isEqualTo(connection2.getServer()); + + connectionManager.close(false); + } + + @Test + public void shouldDestroyConnectionsDoNotGetReturnedToPool() { + Connection connection = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(any())).thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(timeout); + heldConnection.destroy(); + connectionManager.returnConnection(heldConnection, true); + + assertThat(connectionManager.borrowConnection(timeout)).isNotEqualTo(connection); + verify(connectionFactory, times(2)).createClientToServerConnection(any()); + + connectionManager.close(false); + } + + @Test + public void connectionGetsDestroyedWhenReturningToPoolAndOverMaxConnections() { + int maxConnections = 2; + + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Connection connection2 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation2, false)) + .thenReturn(connection2); + + ServerLocation serverLocation3 = mock(ServerLocation.class); + Connection connection3 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation3, false)) + .thenReturn(connection3); + + connectionManager = new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, + maxConnections, 1, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, + cancelCriterion, poolStats); + connectionManager.start(backgroundProcessor); + + Connection heldConnection1 = + connectionManager.borrowConnection(serverLocation1, timeout, false); + Connection heldConnection2 = + connectionManager.borrowConnection(serverLocation2, timeout, false); + Connection heldConnection3 = + connectionManager.borrowConnection(serverLocation3, timeout, false); + + assertThat(connectionManager.getConnectionCount()).isGreaterThan(maxConnections); + + connectionManager.returnConnection(heldConnection3); + assertThat(connectionManager.getConnectionCount()).isEqualTo(maxConnections); + + connectionManager.returnConnection(heldConnection1); + connectionManager.returnConnection(heldConnection2); + assertThat(connectionManager.getConnectionCount()).isEqualTo(maxConnections); + + connectionManager.close(false); + } + + @Test + public void exchangeCreatesNewConnectionIfNoneAreAvailable() { + Set excluded = Collections.emptySet(); + + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Endpoint endpoint2 = mock(Endpoint.class); + Connection connection2 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(eq(Collections.EMPTY_SET))) + .thenReturn(connection2); + when(connection2.getServer()).thenReturn(serverLocation2); + when(connection2.getEndpoint()).thenReturn(endpoint2); + when(endpoint2.getLocation()).thenReturn(serverLocation2); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation1, timeout, false); + heldConnection = connectionManager.exchangeConnection(heldConnection, excluded, timeout); + + assertThat(heldConnection.getServer()).isEqualTo(connection2.getServer()); + assertThat(connectionManager.getConnectionCount()).isEqualTo(2); + verify(connectionFactory, times(1)).createClientToServerConnection(Collections.EMPTY_SET); + + connectionManager.close(false); + } + + @Test + public void exchangeBreaksMaxConnectionContractWhenNoConnectionsAreAvailable() { + int maxConnections = 2; + Set excluded = Collections.emptySet(); + + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Connection connection2 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation2, false)) + .thenReturn(connection2); + + ServerLocation serverLocation3 = mock(ServerLocation.class); + Connection connection3 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation3, false)) + .thenReturn(connection3); + + ServerLocation serverLocation4 = mock(ServerLocation.class); + Endpoint endpoint4 = mock(Endpoint.class); + Connection connection4 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(eq(Collections.EMPTY_SET))) + .thenReturn(connection4); + when(connection4.getServer()).thenReturn(serverLocation4); + when(connection4.getEndpoint()).thenReturn(endpoint4); + when(endpoint4.getLocation()).thenReturn(serverLocation4); + + connectionManager = new ConnectionManagerImpl(poolName, connectionFactory, endpointManager, + maxConnections, 1, idleTimeout, lifetimeTimeout, securityLogger, pingInterval, + cancelCriterion, poolStats); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation1, timeout, false); + connectionManager.borrowConnection(serverLocation2, timeout, false); + connectionManager.borrowConnection(serverLocation3, timeout, false); + assertThat(connectionManager.getConnectionCount()).isGreaterThan(maxConnections); + + heldConnection = connectionManager.exchangeConnection(heldConnection, excluded, timeout); + assertThat(connectionManager.getConnectionCount()).isGreaterThan(maxConnections); + assertThat(heldConnection.getServer()).isEqualTo(connection4.getServer()); + verify(connectionFactory, times(1)).createClientToServerConnection(Collections.EMPTY_SET); + + connectionManager.close(false); + } + + @Test + public void exchangeReturnsExistingConnectionIfOneExists() { + Set excluded = Collections.emptySet(); + + ServerLocation serverLocation1 = mock(ServerLocation.class); + Connection connection1 = mock(Connection.class); + when(connectionFactory.createClientToServerConnection(serverLocation1, false)) + .thenReturn(connection1); + + ServerLocation serverLocation2 = mock(ServerLocation.class); + Connection connection2 = mock(Connection.class); + Endpoint endpoint2 = mock(Endpoint.class); + when(connectionFactory.createClientToServerConnection(serverLocation2, false)) + .thenReturn(connection2); + when(connection2.getServer()).thenReturn(serverLocation2); + when(connection2.getEndpoint()).thenReturn(endpoint2); + when(endpoint2.getLocation()).thenReturn(serverLocation2); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection1 = + connectionManager.borrowConnection(serverLocation1, timeout, false); + Connection heldConnection2 = + connectionManager.borrowConnection(serverLocation2, timeout, false); + + connectionManager.returnConnection(heldConnection2); + heldConnection2 = connectionManager.exchangeConnection(heldConnection1, excluded, timeout); + assertThat(heldConnection2.getServer()).isEqualTo(connection2.getServer()); + assertThat(connectionManager.getConnectionCount()).isEqualTo(2); + + connectionManager.close(false); + } + + @Test + public void activateActivatesConnection() { + Connection connection = mock(Connection.class); + ServerLocation serverLocation = mock(ServerLocation.class); + when(connectionFactory.createClientToServerConnection(serverLocation, false)) + .thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation, timeout, false); + connectionManager.passivate(heldConnection, false); + connectionManager.activate(heldConnection); + + assertThat(((PooledConnection) heldConnection).isActive()).isTrue(); + + connectionManager.close(false); + } + + @Test + public void activateThrowsIfConnectionIsAlreadyActive() { + Connection connection = mock(Connection.class); + ServerLocation serverLocation = mock(ServerLocation.class); + when(connectionFactory.createClientToServerConnection(serverLocation, false)) + .thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation, timeout, false); + + assertThatThrownBy(() -> connectionManager.activate(heldConnection)) + .isInstanceOf(InternalGemFireException.class) + .hasMessageContaining("Connection already active"); + + connectionManager.close(false); + } + + @Test + public void passivatePassivatesConnection() { + Connection connection = mock(Connection.class); + ServerLocation serverLocation = mock(ServerLocation.class); + when(connectionFactory.createClientToServerConnection(serverLocation, false)) + .thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation, timeout, false); + connectionManager.passivate(heldConnection, false); + + assertThat(((PooledConnection) heldConnection).isActive()).isFalse(); + + connectionManager.close(false); + } + + @Test + public void passivateThrowsWhenConnectionIsNotActive() { + Connection connection = mock(Connection.class); + ServerLocation serverLocation = mock(ServerLocation.class); + when(connectionFactory.createClientToServerConnection(serverLocation, false)) + .thenReturn(connection); + + connectionManager = createDefaultConnectionManager(); + connectionManager.start(backgroundProcessor); + + Connection heldConnection = connectionManager.borrowConnection(serverLocation, timeout, false); + connectionManager.passivate(heldConnection, false); + assertThatThrownBy(() -> connectionManager.passivate(heldConnection, false)) + .isInstanceOf(InternalGemFireException.class) + .hasMessageContaining("Connection not active"); + + connectionManager.close(false); + } +} diff --git a/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerJUnitTest.java b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerJUnitTest.java index 15d25cf8d915..9c3b6161176c 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerJUnitTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerJUnitTest.java @@ -570,7 +570,7 @@ public void testExchangeConnection() throws Exception { Assert.assertEquals(0, factory.destroys); Assert.assertEquals(2, manager.getConnectionCount()); - Connection conn3 = manager.exchangeConnection(conn1, Collections.EMPTY_SET, 10); + Connection conn3 = manager.exchangeConnection(conn1, Collections.emptySet(), 10); Assert.assertEquals(3, factory.creates); Assert.assertEquals(1, factory.destroys); diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java index c07493fe2a9d..cf128fdb2749 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java @@ -153,14 +153,12 @@ public Object execute(Op op, int retries) { // while we're performing the op. It will be reset // if the op succeeds. localConnection.set(null); - try { - this.connectionManager.activate(conn); - } catch (ConnectionDestroyedException ex) { + if (!this.connectionManager.activate(conn)) { conn = connectionManager.borrowConnection(serverTimeout); } } try { - Set attemptedServers = null; + Set attemptedServers = null; for (int attempt = 0; true; attempt++) { // when an op is retried we may need to try to recover the previous @@ -183,7 +181,7 @@ public Object execute(Op op, int retries) { handleException(e, conn, attempt, attempt >= retries && retries != -1); if (null == attemptedServers) { // don't allocate this until we need it - attemptedServers = new HashSet(); + attemptedServers = new HashSet<>(); } attemptedServers.add(conn.getServer()); try { @@ -445,15 +443,13 @@ private Connection getActivatedThreadLocalConnectionForSingleHop(ServerLocation } boolean borrow = true; if (conn != null) { - try { - this.connectionManager.activate(conn); + if (this.connectionManager.activate(conn)) { borrow = false; if (!conn.getServer().equals(server)) { // poolLoadConditioningMonitor can replace the connection's // endpoint from underneath us. fixes bug 45151 borrow = true; } - } catch (ConnectionDestroyedException e) { } } if (conn == null || borrow) { diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java new file mode 100644 index 000000000000..6628266f4c6c --- /dev/null +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java @@ -0,0 +1,143 @@ +/* + * 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.cache.client.internal.pooling; + +import java.util.Deque; +import java.util.concurrent.ConcurrentLinkedDeque; +import java.util.function.Predicate; + +/** + * This manager maintains a collection of PooledConnection instances. + * PooledConnections can be added to this manager using one of the add* methods. + * PooledConnections can be taken out of this manager to be used by calling one of the use* methods. + * Once a PooledConnection has reached the end of its life it can be permanently removed from this + * manager using the remove method. + * The manager has a concept of "first" which identifies the PooledConnections that should be + * preferred to be used by the next use* method, and "last" which identifies PooledConnections that + * should only be used as a last resort. + * + */ +public class AvailableConnectionManager { + private final Deque connections = + new ConcurrentLinkedDeque<>(); + + /** + * Remove, activate, and return the first connection. + * Connections that can not be activated will be removed from the manager but not returned. + * + * @return the activated connection or null if none found + */ + public PooledConnection useFirst() { + PooledConnection connection; + while (null != (connection = connections.pollFirst())) { + if (connection.activate()) { + return connection; + } + } + return null; + } + + /** + * Removes the first connection equal to the given connection from this manager. + * + * @param connection the connection to remove + * @return true if a connection was removed; otherwise false + */ + public boolean remove(PooledConnection connection) { + return connections.remove(connection); + } + + /** + * Remove, activate, and return the first connection that matches the predicate. + * Connections that can not be activated will be removed from the manager but not returned. + * + * @param predicate that the connections are matched against + * @return the activated connection or null if none found + */ + public PooledConnection useFirst(Predicate predicate) { + final EqualsWithPredicate equalsWithPredicate = new EqualsWithPredicate(predicate); + while (connections.removeFirstOccurrence(equalsWithPredicate)) { + PooledConnection connection = equalsWithPredicate.getConnectionThatMatched(); + if (connection.activate()) { + return connection; + } + } + return null; + } + + /** + * Passivate and add the given connection to this manager as its first, highest priority + * connection. + * + * @param connection the connection to passivate and add + * @param accessed true if the connection was used by the caller, false otherwise + */ + public void addFirst(PooledConnection connection, boolean accessed) { + passivate(connection, accessed); + connections.addFirst(connection); + } + + /** + * Passivate and add the given connection to this manager as its last, lowest priority connection. + * + * @param connection the connection to passivate and add + * @param accessed true if the connection was used by the caller, false otherwise + */ + public void addLast(PooledConnection connection, boolean accessed) { + passivate(connection, accessed); + connections.addLast(connection); + } + + private void passivate(PooledConnection connection, boolean accessed) { + // thread local connections are already passive at this point + if (connection.isActive()) { + connection.passivate(accessed); + } + } + + // used by unit tests + Deque getDeque() { + return connections; + } + + /** + * This class exists so that we can use ConcurrentLinkedDeque removeFirstOccurrence. + * We want to efficiently scan the Deque for an item that matches the predicate without changing + * the position of items that do not match. We also need to know the identity of the first item + * that did match. + */ + private static class EqualsWithPredicate { + private final Predicate predicate; + private PooledConnection connectionThatMatched; + + EqualsWithPredicate(Predicate predicate) { + this.predicate = predicate; + } + + @Override + public boolean equals(Object o) { + PooledConnection pooledConnection = (PooledConnection) o; + if (predicate.test(pooledConnection)) { + this.connectionThatMatched = pooledConnection; + return true; + } + return false; + } + + public PooledConnection getConnectionThatMatched() { + return this.connectionThatMatched; + } + } +} diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccounting.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccounting.java new file mode 100644 index 000000000000..3965f7928e6a --- /dev/null +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccounting.java @@ -0,0 +1,148 @@ +/* + * 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.cache.client.internal.pooling; + +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Responsible for counting connections. + * The count maintained by this class will eventually be consistent with the actual number of + * connections. Since the count is changed before and after the actual connections are created and + * destroyed, and not changed while holding a lock, the count should be treated as an estimate of + * the current number of connections. + */ +public class ConnectionAccounting { + private final int minimum; + private final int maximum; + private final AtomicInteger count = new AtomicInteger(); + + public ConnectionAccounting(int min, int max) { + this.minimum = min; + this.maximum = max; + } + + public int getMinimum() { + return minimum; + } + + public int getMaximum() { + return maximum; + } + + public int getCount() { + return count.get(); + } + + /** + * Should be called when prefilling connections to reach minimum connections. Caller should only + * create a connection if this method returns {@code true}. If connection creation fails then + * {@link #cancelTryPrefill} must be called to revert the count increase. + * + * @return {@code true} if count was under minimum and we increased it, otherwise {@code false}. + */ + public boolean tryPrefill() { + return tryReserve(minimum); + } + + /** + * Should only be called if connection creation failed after calling {@link #tryPrefill()} ()}. + */ + public void cancelTryPrefill() { + count.getAndDecrement(); + } + + /** + * Should be called when a new connection would be nice to have when count is under maximum. + * Caller should only create a connection if this method returns {@code true}. If connection + * creation fails then {@link #cancelTryCreate} must be called to revert the count increase. + * + * @return {@code true} if count was under maximum and we increased it, otherwise {@code false}. + */ + public boolean tryCreate() { + return tryReserve(maximum); + } + + /** + * Should only be called if connection creation failed after calling {@link #tryCreate()}. + */ + public void cancelTryCreate() { + count.decrementAndGet(); + } + + /** + * Count a created connection regardless of maximum. Should not be called after + * {@link #tryCreate()}. + */ + public void create() { + count.getAndIncrement(); + } + + /** + * Should be called when a connection is being returned and the caller should destroy the + * connection if {@code true} is returned. If connection destroy fails then + * {@link #cancelTryDestroy()} must be called. + * + * @return {@code true} if count was over maximum and we decreased it, otherwise {@code false}. + */ + public boolean tryDestroy() { + int currentCount; + while ((currentCount = count.get()) > maximum) { + if (count.compareAndSet(currentCount, currentCount - 1)) { + return true; + } + } + return false; + } + + /** + * Should only be called if connection destroy failed after calling {@link #tryDestroy()}. + */ + public void cancelTryDestroy() { + count.getAndIncrement(); + } + + /** + * Should be called after any connection destroys are done. Should not be called + * after {@link #tryDestroy()}. + * + * @param destroyCount number of connections being destroyed. + * + * @return {@code true} if after decreasing count it is under the minimum, otherwise + * {@code false}. + */ + public boolean destroyAndIsUnderMinimum(int destroyCount) { + int newCount = count.addAndGet(-destroyCount); + return newCount < minimum; + } + + public boolean isUnderMinimum() { + return count.get() < minimum; + } + + public boolean isOverMinimum() { + return count.get() > minimum; + } + + private boolean tryReserve(int upperBound) { + int currentCount; + while ((currentCount = count.get()) < upperBound) { + if (count.compareAndSet(currentCount, currentCount + 1)) { + return true; + } + } + return false; + } +} diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java index 55ddac076466..ea221f018ef4 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManager.java @@ -17,9 +17,13 @@ import java.util.Set; import java.util.concurrent.ScheduledExecutorService; +import org.apache.geode.InternalGemFireException; import org.apache.geode.cache.client.AllConnectionsInUseException; import org.apache.geode.cache.client.NoAvailableServersException; +import org.apache.geode.cache.client.ServerConnectivityException; +import org.apache.geode.cache.client.ServerOperationException; import org.apache.geode.cache.client.internal.Connection; +import org.apache.geode.distributed.PoolCancelledException; import org.apache.geode.distributed.internal.ServerLocation; /** @@ -32,26 +36,35 @@ public interface ConnectionManager { /** - * Borrow an existing idle connection or create a new one. + * Borrow an existing idle connection or create a new one. Fails after one failed attempt to + * create a new connection. * * @param aquireTimeout The amount of time to wait for a connection to become available. * @return A connection to use. - * @throws AllConnectionsInUseException If the maximum number of connections are already in use + * @throws AllConnectionsInUseException If the maximum number of connections are already, in use * and no connection becomes free within the aquireTimeout. * @throws NoAvailableServersException if we can't connect to any server + * @throws ServerOperationException if there is an issue with security or connecting to a gateway + * @throws PoolCancelledException if the pool is being shut down */ Connection borrowConnection(long aquireTimeout) throws AllConnectionsInUseException, NoAvailableServersException; /** - * Borrow an existing idle connection or create a new one to a specific server. + * Borrow an existing idle connection or create a new one to a specific server. Fails after one + * failed attempt to create a new connection. May cause pool to exceed maxConnections by one, if + * no connection is available. * * @param server The server the connection needs to be to. * @param aquireTimeout The amount of time to wait for a connection to become available. + * @param onlyUseExistingCnx if true, will not create a new connection if none are available. * @return A connection to use. - * @throws AllConnectionsInUseException If the maximum number of connections are already in use - * and no connection becomes free within the aquireTimeout. - * @throws NoAvailableServersException if we can't connect to any server + * @throws AllConnectionsInUseException If there is no available connection on the desired server, + * and onlyUseExistingCnx is set. + * @throws ServerOperationException If there is an issue creating the connection due to security + * @throws NoAvailableServersException If we can't connect to any server + * @throws ServerConnectivityException If finding a connection and creating a connection both fail + * to return a connection * */ Connection borrowConnection(ServerLocation server, long aquireTimeout, boolean onlyUseExistingCnx) @@ -85,14 +98,20 @@ Connection borrowConnection(ServerLocation server, long aquireTimeout, boolean o void close(boolean keepAlive); /** - * Exchange one connection for a new connection to a different server. + * Exchange one connection for a new connection to a different server. This method can break the + * max connection contract if there is no available connection and maxConnections has already been + * reached. * * @param conn connection to exchange. It will be returned to the pool (or destroyed if it has * been invalidated). * @param excludedServers servers to exclude when looking for a new connection + * @param aquireTimeout The amount of time to wait for a connection to be available + * @throws InternalGemFireException if the found connection is already active + * @throws NoAvailableServersException if no servers are available to connect to + * @throws ServerOperationException if creating a connection fails due to authentication issues * @return a new connection to the pool to a server that is not in the list of excluded servers */ - Connection exchangeConnection(Connection conn, Set/* */ excludedServers, + Connection exchangeConnection(Connection conn, Set excludedServers, long aquireTimeout) throws AllConnectionsInUseException; /** @@ -104,11 +123,16 @@ Connection exchangeConnection(Connection conn, Set/* */ exclude /** * Used to active a thread local connection + * + * @return true if connection activated, false if could not be activated because it is destroyed + * @throws InternalGemFireException when the connection is already active */ - void activate(Connection conn); + boolean activate(Connection conn); /** * Used to passivate a thread local connection + * + * @throws InternalGemFireException when the connection is already passive */ void passivate(Connection conn, boolean accessed); diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java index ae0b4815b06c..fff52e319a6e 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/ConnectionManagerImpl.java @@ -14,8 +14,9 @@ */ package org.apache.geode.cache.client.internal.pooling; +import static java.util.concurrent.TimeUnit.MILLISECONDS; + import java.net.SocketException; -import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -24,13 +25,13 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.SplittableRandom; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.Condition; -import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.logging.log4j.Logger; @@ -38,7 +39,6 @@ import org.apache.geode.CancelException; import org.apache.geode.SystemFailure; import org.apache.geode.cache.CacheClosedException; -import org.apache.geode.cache.GatewayConfigurationException; import org.apache.geode.cache.client.AllConnectionsInUseException; import org.apache.geode.cache.client.NoAvailableServersException; import org.apache.geode.cache.client.ServerConnectivityException; @@ -70,15 +70,15 @@ */ public class ConnectionManagerImpl implements ConnectionManager { private static final Logger logger = LogService.getLogger(); + private static final int NOT_WAITING = -1; private final String poolName; private final PoolStats poolStats; protected final long prefillRetry; // ms - private final ArrayDeque availableConnections = new ArrayDeque<>(); + private final AvailableConnectionManager availableConnectionManager = + new AvailableConnectionManager(); protected final ConnectionMap allConnectionsMap = new ConnectionMap(); private final EndpointManager endpointManager; - private final int maxConnections; - protected final int minConnections; private final long idleTimeout; // make this an int protected final long idleTimeoutNanos; final int lifetimeTimeout; @@ -86,21 +86,17 @@ public class ConnectionManagerImpl implements ConnectionManager { private final InternalLogWriter securityLogWriter; protected final CancelCriterion cancelCriterion; - protected volatile int connectionCount; + private final ConnectionAccounting connectionAccounting; protected ScheduledExecutorService backgroundProcessor; protected ScheduledExecutorService loadConditioningProcessor; - protected ReentrantLock lock = new ReentrantLock(); - protected Condition freeConnection = lock.newCondition(); private ConnectionFactory connectionFactory; protected boolean haveIdleExpireConnectionsTask; - protected boolean havePrefillTask; + protected final AtomicBoolean havePrefillTask = new AtomicBoolean(false); private boolean keepAlive = false; - protected volatile boolean shuttingDown; + protected final AtomicBoolean shuttingDown = new AtomicBoolean(false); private EndpointManager.EndpointListenerAdapter endpointListener; - private static final long NANOS_PER_MS = 1000000L; - /** * Adds an arbitrary variance to a positive temporal interval. Where possible, 10% of the interval * is added or subtracted from the interval. Otherwise, 1 is added or subtracted from the @@ -153,10 +149,10 @@ public ConnectionManagerImpl(String poolName, ConnectionFactory factory, this.connectionFactory = factory; this.endpointManager = endpointManager; - this.maxConnections = maxConnections == -1 ? Integer.MAX_VALUE : maxConnections; - this.minConnections = minConnections; + this.connectionAccounting = new ConnectionAccounting(minConnections, + maxConnections == -1 ? Integer.MAX_VALUE : maxConnections); this.lifetimeTimeout = addVarianceToInterval(lifetimeTimeout); - this.lifetimeTimeoutNanos = this.lifetimeTimeout * NANOS_PER_MS; + this.lifetimeTimeoutNanos = MILLISECONDS.toNanos(this.lifetimeTimeout); if (this.lifetimeTimeout != -1) { if (idleTimeout > this.lifetimeTimeout || idleTimeout == -1) { // lifetimeTimeout takes precedence over longer idle timeouts @@ -164,7 +160,7 @@ public ConnectionManagerImpl(String poolName, ConnectionFactory factory, } } this.idleTimeout = idleTimeout; - this.idleTimeoutNanos = this.idleTimeout * NANOS_PER_MS; + this.idleTimeoutNanos = MILLISECONDS.toNanos(this.idleTimeout); this.securityLogWriter = securityLogger; this.prefillRetry = pingInterval; this.cancelCriterion = cancelCriterion; @@ -176,96 +172,135 @@ public void endpointCrashed(Endpoint endpoint) { }; } - /* - * (non-Javadoc) - * - * @see org.apache.geode.cache.client.internal.pooling.ConnectionManager#borrowConnection(long) - */ - @Override - public Connection borrowConnection(long acquireTimeout) - throws AllConnectionsInUseException, NoAvailableServersException { + private void destroyAndMaybePrefill() { + destroyAndMaybePrefill(1); + } + + private void destroyAndMaybePrefill(int count) { + if (connectionAccounting.destroyAndIsUnderMinimum(count)) { + startBackgroundPrefill(); + } + } - long startTime = System.currentTimeMillis(); - long remainingTime = acquireTimeout; + private PooledConnection createPooledConnection() + throws NoAvailableServersException, ServerOperationException { + return createPooledConnection(Collections.emptySet()); + } - // wait for a connection to become free - lock.lock(); + private PooledConnection createPooledConnection(Set excludedServers) + throws NoAvailableServersException, ServerOperationException { try { - while (connectionCount >= maxConnections && availableConnections.isEmpty() - && remainingTime > 0 && !shuttingDown) { - final long start = getPoolStats().beginConnectionWait(); - boolean interrupted = false; - try { - freeConnection.await(remainingTime, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - interrupted = true; - cancelCriterion.checkCancelInProgress(e); - throw new AllConnectionsInUseException(); - } finally { - if (interrupted) { - Thread.currentThread().interrupt(); - } - getPoolStats().endConnectionWait(start); - } - remainingTime = acquireTimeout - (System.currentTimeMillis() - startTime); - } - if (shuttingDown) { - throw new PoolCancelledException(); - } + return addConnection(connectionFactory.createClientToServerConnection(excludedServers)); + } catch (GemFireSecurityException e) { + throw new ServerOperationException(e); + } catch (ServerRefusedConnectionException e) { + throw new NoAvailableServersException(e); + } + } - while (!availableConnections.isEmpty()) { - PooledConnection connection = availableConnections.removeFirst(); - try { - connection.activate(); - return connection; - } catch (ConnectionDestroyedException ex) { - // whoever destroyed it already decremented connectionCount - } - } - if (connectionCount >= maxConnections) { - throw new AllConnectionsInUseException(); - } else { - // We need to create a connection. Reserve space for it. - connectionCount++; - } + private PooledConnection createPooledConnection(ServerLocation serverLocation) + throws ServerRefusedConnectionException, GemFireSecurityException { + return addConnection(connectionFactory.createClientToServerConnection(serverLocation, false)); + } - } finally { - lock.unlock(); + /** + * Always creates a connection and may cause {@link #connectionCount} to exceed + * {@link #maxConnections}. + */ + private PooledConnection forceCreateConnection(ServerLocation serverLocation) + throws ServerRefusedConnectionException, ServerOperationException { + connectionAccounting.create(); + try { + return createPooledConnection(serverLocation); + } catch (GemFireSecurityException e) { + throw new ServerOperationException(e); + } + } + + /** + * Always creates a connection and may cause {@link #connectionCount} to exceed + * {@link #maxConnections}. + */ + private PooledConnection forceCreateConnection(Set excludedServers) + throws NoAvailableServersException, ServerOperationException { + connectionAccounting.create(); + return createPooledConnection(excludedServers); + } + + private boolean checkShutdownInterruptedOrTimeout(final long timeout) + throws PoolCancelledException { + if (shuttingDown.get()) { + throw new PoolCancelledException(); + } + + if (Thread.currentThread().isInterrupted()) { + return true; + } + + if (timeout < System.nanoTime()) { + return true; } - PooledConnection connection = null; + return false; + } + + private long beginConnectionWaitStatIfNotStarted(final long waitStart) { + if (NOT_WAITING == waitStart) { + return getPoolStats().beginConnectionWait(); + } + + return waitStart; + } + + private void endConnectionWaitStatIfStarted(final long waitStart) { + if (NOT_WAITING != waitStart) { + getPoolStats().endConnectionWait(waitStart); + } + } + + @Override + public Connection borrowConnection(long acquireTimeout) + throws AllConnectionsInUseException, NoAvailableServersException, ServerOperationException { + long waitStart = NOT_WAITING; try { - Connection plainConnection = - connectionFactory.createClientToServerConnection(Collections.EMPTY_SET); + long timeout = System.nanoTime() + MILLISECONDS.toNanos(acquireTimeout); + while (true) { + PooledConnection connection = availableConnectionManager.useFirst(); + if (null != connection) { + return connection; + } - connection = addConnection(plainConnection); - } catch (GemFireSecurityException e) { - throw new ServerOperationException(e); - } catch (GatewayConfigurationException e) { - throw new ServerOperationException(e); - } catch (ServerRefusedConnectionException srce) { - throw new NoAvailableServersException(srce); - } finally { - // if we failed, release the space we reserved for our connection - if (connection == null) { - lock.lock(); - try { - --connectionCount; - if (connectionCount < minConnections) { - startBackgroundPrefill(); + if (connectionAccounting.tryCreate()) { + try { + connection = createPooledConnection(); + if (null != connection) { + return connection; + } + throw new NoAvailableServersException(); + } finally { + if (connection == null) { + connectionAccounting.cancelTryCreate(); + if (connectionAccounting.isUnderMinimum()) { + startBackgroundPrefill(); + } + } } - } finally { - lock.unlock(); } - } - } - if (connection == null) { - this.cancelCriterion.checkCancelInProgress(null); - throw new NoAvailableServersException(); + if (checkShutdownInterruptedOrTimeout(timeout)) { + break; + } + + waitStart = beginConnectionWaitStatIfNotStarted(waitStart); + + Thread.yield(); + } + } finally { + endConnectionWaitStatIfStarted(waitStart); } - return connection; + this.cancelCriterion.checkCancelInProgress(null); + throw new AllConnectionsInUseException(); } /** @@ -277,151 +312,49 @@ public Connection borrowConnection(long acquireTimeout) @Override public Connection borrowConnection(ServerLocation server, long acquireTimeout, boolean onlyUseExistingCnx) throws AllConnectionsInUseException, NoAvailableServersException { - lock.lock(); - try { - if (shuttingDown) { - throw new PoolCancelledException(); - } - for (Iterator itr = availableConnections.iterator(); itr.hasNext();) { - PooledConnection nextConnection = itr.next(); - try { - nextConnection.activate(); - if (nextConnection.getServer().equals(server)) { - itr.remove(); - return nextConnection; - } - nextConnection.passivate(false); - } catch (ConnectionDestroyedException ex) { - // someone else already destroyed this connection so ignore it - // but remove it from availableConnections - } - // Fix for 41516. Before we let this method exceed the max connections - // by creating a new connection, we need to make sure that they're - // aren't bogus connections sitting in the available connection list - // otherwise, the length of that list might exceed max connections, - // but with some bad connections. That can cause members to - // get a bad connection but have no permits to create a new connection. - if (nextConnection.shouldDestroy()) { - itr.remove(); - } - } - - if (onlyUseExistingCnx) { - throw new AllConnectionsInUseException(); - } - - // We need to create a connection. Reserve space for it. - connectionCount++; - } finally { - lock.unlock(); + PooledConnection connection = + availableConnectionManager.useFirst((c) -> c.getServer().equals(server)); + if (null != connection) { + return connection; } - PooledConnection connection = null; - try { - Connection plainConnection = connectionFactory.createClientToServerConnection(server, false); - connection = addConnection(plainConnection); - } catch (GemFireSecurityException e) { - throw new ServerOperationException(e); - } finally { - // if we failed, release the space we reserved for our connection - if (connection == null) { - lock.lock(); - try { - --connectionCount; - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } - } finally { - lock.unlock(); - } - } + if (onlyUseExistingCnx) { + throw new AllConnectionsInUseException(); } - if (connection == null) { - throw new ServerConnectivityException( - "Could not create a new connection to server " + server); + + connection = forceCreateConnection(server); + if (null != connection) { + return connection; } - return connection; + + throw new ServerConnectivityException( + "Could not create a new connection to server " + server); } @Override - public Connection exchangeConnection(Connection oldConnection, - Set/* */ excludedServers, long acquireTimeout) + public Connection exchangeConnection(final Connection oldConnection, + final Set excludedServers, final long acquireTimeout) throws AllConnectionsInUseException { - assert oldConnection instanceof PooledConnection; - PooledConnection newConnection = null; - PooledConnection oldPC = (PooledConnection) oldConnection; - boolean needToUndoEstimate = false; - lock.lock(); try { - if (shuttingDown) { - throw new PoolCancelledException(); + PooledConnection connection = availableConnectionManager + .useFirst((c) -> !excludedServers.contains(c.getServer())); + if (null != connection) { + return connection; } - for (Iterator itr = availableConnections.iterator(); itr.hasNext();) { - PooledConnection nextConnection = itr.next(); - if (!excludedServers.contains(nextConnection.getServer())) { - itr.remove(); - try { - nextConnection.activate(); - newConnection = nextConnection; - if (allConnectionsMap.removeConnection(oldPC)) { - --connectionCount; - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } - } - break; - } catch (ConnectionDestroyedException ex) { - // someone else already destroyed this connection so ignore it - // but remove it from availableConnections - } - } - } - if (newConnection == null) { - if (!allConnectionsMap.removeConnection(oldPC)) { - // We need to create a connection. Reserve space for it. - needToUndoEstimate = true; - connectionCount++; - } - } - } finally { - lock.unlock(); - } - if (newConnection == null) { - try { - Connection plainConnection = - connectionFactory.createClientToServerConnection(excludedServers); - newConnection = addConnection(plainConnection); - } catch (GemFireSecurityException e) { - throw new ServerOperationException(e); - } catch (ServerRefusedConnectionException srce) { - throw new NoAvailableServersException(srce); - } finally { - if (needToUndoEstimate && newConnection == null) { - lock.lock(); - try { - --connectionCount; - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } - } finally { - lock.unlock(); - } - } + connection = forceCreateConnection(excludedServers); + if (null != connection) { + return connection; } - } - if (newConnection == null) { throw new NoAvailableServersException(); + } finally { + returnConnection(oldConnection, true, true); } - - oldPC.internalDestroy(); - - return newConnection; } - protected/* GemStoneAddition */ String getPoolName() { + protected String getPoolName() { return this.poolName; } @@ -437,92 +370,47 @@ private PooledConnection addConnection(Connection conn) { allConnectionsMap.addConnection(pooledConn); if (logger.isDebugEnabled()) { logger.debug("Created a new connection. {} Connection count is now {}", pooledConn, - connectionCount); + connectionAccounting.getCount()); } return pooledConn; } private void destroyConnection(PooledConnection connection) { - lock.lock(); - try { - if (allConnectionsMap.removeConnection(connection)) { - if (logger.isDebugEnabled()) { - logger.debug("Invalidating connection {} connection count is now {}", connection, - connectionCount); - } - - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } - freeConnection.signalAll(); + if (allConnectionsMap.removeConnection(connection)) { + if (logger.isDebugEnabled()) { + logger.debug("Invalidating connection {} connection count is now {}", connection, + connectionAccounting.getCount()); } - --connectionCount; // fix for bug #50333 - } finally { - lock.unlock(); + + destroyAndMaybePrefill(); } connection.internalDestroy(); } - /* - * (non-Javadoc) - * - * @see - * org.apache.geode.cache.client.internal.pooling.ConnectionManager#invalidateServer(org.apache. - * geode.distributed.internal.ServerLocation) - */ protected void invalidateServer(Endpoint endpoint) { - Set badConnections = allConnectionsMap.removeEndpoint(endpoint); + Set badConnections = allConnectionsMap.removeEndpoint(endpoint); if (badConnections == null) { return; } - lock.lock(); - try { - if (shuttingDown) { - return; - } - if (logger.isDebugEnabled()) { - logger.debug("Invalidating {} connections to server {}", badConnections.size(), endpoint); - } - - // mark connections for destruction now, so if anyone tries - // to return a connection they'll get an exception - for (Iterator itr = badConnections.iterator(); itr.hasNext();) { - PooledConnection conn = (PooledConnection) itr.next(); - if (!conn.setShouldDestroy()) { - } - } - - availableConnections.removeIf(badConnections::contains); - - connectionCount -= badConnections.size(); - - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } + if (shuttingDown.get()) { + return; + } - for (Iterator itr = badConnections.iterator(); itr.hasNext();) { - PooledConnection conn = (PooledConnection) itr.next(); - conn.internalDestroy(); - } + if (logger.isDebugEnabled()) { + logger.debug("Invalidating {} connections to server {}", badConnections.size(), endpoint); + } - if (connectionCount < maxConnections) { - freeConnection.signalAll(); + for (PooledConnection conn : badConnections) { + if (conn.internalDestroy()) { + destroyAndMaybePrefill(); + availableConnectionManager.remove(conn); } - } finally { - lock.unlock(); } } - /* - * (non-Javadoc) - * - * @see - * org.apache.geode.cache.client.internal.pooling.ConnectionManager#returnConnection(org.apache. - * geode.cache.client.internal.Connection) - */ @Override public void returnConnection(Connection connection) { returnConnection(connection, true); @@ -530,83 +418,69 @@ public void returnConnection(Connection connection) { @Override public void returnConnection(Connection connection, boolean accessed) { + returnConnection(connection, accessed, false); + } + private void returnConnection(Connection connection, boolean accessed, boolean addLast) { assert connection instanceof PooledConnection; PooledConnection pooledConn = (PooledConnection) connection; - boolean shouldClose = false; - - lock.lock(); - try { - if (pooledConn.isDestroyed()) { - return; - } + if (pooledConn.isDestroyed()) { + return; + } - if (pooledConn.shouldDestroy()) { - destroyConnection(pooledConn); + if (pooledConn.shouldDestroy()) { + destroyConnection(pooledConn); + } else if (!destroyIfOverLimit(pooledConn)) { + if (addLast) { + availableConnectionManager.addLast(pooledConn, accessed); } else { - // thread local connections are already passive at this point - if (pooledConn.isActive()) { - pooledConn.passivate(accessed); - } - - // borrowConnection(ServerLocation, long) allows us to break the - // connection limit in order to get a connection to a server. So we need - // to get our pool back to size if we're above the limit - if (connectionCount > maxConnections) { - if (allConnectionsMap.removeConnection(pooledConn)) { - shouldClose = true; - // getPoolStats().incConCount(-1); - --connectionCount; - } - } else { - availableConnections.addFirst(pooledConn); - freeConnection.signalAll(); - } + availableConnectionManager.addFirst(pooledConn, accessed); } - } finally { - lock.unlock(); } + } - if (shouldClose) { - try { - PoolImpl localpool = (PoolImpl) PoolManagerImpl.getPMI().find(poolName); - boolean durable = false; - if (localpool != null) { - durable = localpool.isDurableClient(); + /** + * Destroys connection if and only if {@link #connectionCount} exceeds {@link #maxConnections}. + * + * @return true if connection is destroyed, otherwise false. + */ + private boolean destroyIfOverLimit(PooledConnection connection) { + if (connectionAccounting.tryDestroy()) { + if (allConnectionsMap.removeConnection(connection)) { + try { + PoolImpl localpool = (PoolImpl) PoolManagerImpl.getPMI().find(poolName); + boolean durable = false; + if (localpool != null) { + durable = localpool.isDurableClient(); + } + connection.internalClose(durable || this.keepAlive); + } catch (Exception e) { + logger.warn(String.format("Error closing connection %s", connection), e); } - pooledConn.internalClose(durable || this.keepAlive); - } catch (Exception e) { - logger.warn(String.format("Error closing connection %s", pooledConn), e); + } else { + // Not a pooled connection so undo the decrement. + connectionAccounting.cancelTryDestroy(); } + + return true; } + + return false; } - /* - * (non-Javadoc) - */ @Override public void start(ScheduledExecutorService backgroundProcessor) { this.backgroundProcessor = backgroundProcessor; String name = "poolLoadConditioningMonitor-" + getPoolName(); this.loadConditioningProcessor = - LoggingExecutors.newScheduledThreadPool(name, 1/* why not 0? */, false); + LoggingExecutors.newScheduledThreadPool(name, 1, false); endpointManager.addListener(endpointListener); - lock.lock(); - try { - startBackgroundPrefill(); - } finally { - lock.unlock(); - } + startBackgroundPrefill(); } - /* - * (non-Javadoc) - * - * @see org.apache.geode.cache.client.internal.pooling.ConnectionManager#close(boolean, long) - */ @Override public void close(boolean keepAlive) { if (logger.isDebugEnabled()) { @@ -615,21 +489,15 @@ public void close(boolean keepAlive) { this.keepAlive = keepAlive; endpointManager.removeListener(endpointListener); - lock.lock(); - try { - if (shuttingDown) { - return; - } - shuttingDown = true; - } finally { - lock.unlock(); + if (!shuttingDown.compareAndSet(false, true)) { + return; } try { if (this.loadConditioningProcessor != null) { this.loadConditioningProcessor.shutdown(); if (!this.loadConditioningProcessor.awaitTermination(PoolImpl.SHUTDOWN_TIMEOUT, - TimeUnit.MILLISECONDS)) { + MILLISECONDS)) { logger.warn("Timeout waiting for load conditioning tasks to complete"); } } @@ -645,7 +513,7 @@ public void close(boolean keepAlive) { @Override public void emergencyClose() { - shuttingDown = true; + shuttingDown.set(true); if (this.loadConditioningProcessor != null) { this.loadConditioningProcessor.shutdown(); } @@ -659,7 +527,7 @@ protected void startBackgroundExpiration() { haveIdleExpireConnectionsTask = true; try { backgroundProcessor.schedule(new IdleExpireConnectionsTask(), idleTimeout, - TimeUnit.MILLISECONDS); + MILLISECONDS); } catch (RejectedExecutionException e) { // ignore, the timer has been cancelled, which means we're shutting // down. @@ -669,10 +537,8 @@ protected void startBackgroundExpiration() { } } - /** Always called with lock held */ protected void startBackgroundPrefill() { - if (!havePrefillTask) { - havePrefillTask = true; + if (havePrefillTask.compareAndSet(false, true)) { try { backgroundProcessor.execute(new PrefillConnectionsTask()); } catch (RejectedExecutionException e) { @@ -684,7 +550,7 @@ protected void startBackgroundPrefill() { protected boolean prefill() { try { - while (connectionCount < minConnections) { + while (connectionAccounting.isUnderMinimum()) { if (cancelCriterion.isCancelInProgress()) { return true; } @@ -707,7 +573,7 @@ protected boolean prefill() { @Override public int getConnectionCount() { - return this.connectionCount; + return connectionAccounting.getCount(); } protected PoolStats getPoolStats() { @@ -725,61 +591,43 @@ public Connection getConnection(Connection conn) { } } + private boolean prefillConnection() { - boolean createConnection = false; - lock.lock(); - try { - if (shuttingDown) { - return false; - } - if (connectionCount < minConnections) { - connectionCount++; - createConnection = true; - } - } finally { - lock.unlock(); + if (shuttingDown.get()) { + return false; } - if (createConnection) { + if (connectionAccounting.tryPrefill()) { PooledConnection connection = null; try { - Connection plainConnection = - connectionFactory.createClientToServerConnection(Collections.EMPTY_SET); - if (plainConnection == null) { + connection = createPooledConnection(); + if (connection == null) { return false; } - connection = addConnection(plainConnection); - connection.passivate(false); getPoolStats().incPrefillConnect(); + availableConnectionManager.addLast(connection, false); + if (logger.isDebugEnabled()) { + logger.debug("Prefilled connection {} connection count is now {}", connection, + connectionAccounting.getCount()); + } + return true; } catch (ServerConnectivityException ex) { - logger - .info(String.format("Unable to prefill pool to minimum because: %s", - ex.getMessage())); + logger.info( + String.format("Unable to prefill pool to minimum because: %s", ex.getMessage())); return false; } finally { - lock.lock(); - try { - if (connection == null) { - connectionCount--; - if (logger.isDebugEnabled()) { - logger.debug("Unable to prefill pool to minimum, connection count is now {}", - connectionCount); - } - } else { - availableConnections.addFirst(connection); - freeConnection.signalAll(); - if (logger.isDebugEnabled()) { - logger.debug("Prefilled connection {} connection count is now {}", connection, - connectionCount); - } + if (connection == null) { + connectionAccounting.cancelTryPrefill(); + + if (logger.isDebugEnabled()) { + logger.debug("Unable to prefill pool to minimum, connection count is now {}", + this::getConnectionCount); } - } finally { - lock.unlock(); } } } - return true; + return false; } public static void loadEmergencyClasses() { @@ -810,7 +658,7 @@ protected class IdleExpireConnectionsTask implements Runnable { public void run() { try { getPoolStats().incIdleCheck(); - allConnectionsMap.checkIdleExpiration(); + allConnectionsMap.expireIdleConnections(); } catch (CancelException ignore) { } catch (VirtualMachineError e) { SystemFailure.initiateFailure(e); @@ -827,28 +675,21 @@ public void run() { } protected class PrefillConnectionsTask extends PoolTask { - @Override public void run2() { if (logger.isTraceEnabled()) { logger.trace("Prefill Connections task running"); } - prefill(); - lock.lock(); - try { - if (connectionCount < minConnections && !cancelCriterion.isCancelInProgress()) { - try { - backgroundProcessor.schedule(new PrefillConnectionsTask(), prefillRetry, - TimeUnit.MILLISECONDS); - } catch (RejectedExecutionException e) { - // ignore, the timer has been cancelled, which means we're shutting down. - } - } else { - havePrefillTask = false; + if (connectionAccounting.isUnderMinimum() && !cancelCriterion.isCancelInProgress()) { + try { + backgroundProcessor.schedule(new PrefillConnectionsTask(), prefillRetry, + MILLISECONDS); + } catch (RejectedExecutionException ignored) { + // ignore, the timer has been cancelled, which means we're shutting down. } - } finally { - lock.unlock(); + } else { + havePrefillTask.set(false); } } } @@ -902,66 +743,54 @@ private boolean offerReplacementConnection(Connection con, ServerLocation curren * @return true if caller should recheck for expired lifetimes; false if a background check was * scheduled or no expirations are possible. */ - public boolean createLifetimeReplacementConnection(ServerLocation currentServer, + private boolean createLifetimeReplacementConnection(ServerLocation currentServer, boolean idlePossible) { - HashSet excludedServers = new HashSet(); - ServerLocation sl = this.connectionFactory.findBestServer(currentServer, excludedServers); - - while (sl != null) { - if (sl.equals(currentServer)) { - this.allConnectionsMap.extendLifeOfCnxToServer(currentServer); + HashSet excludedServers = new HashSet<>(); + while (true) { + ServerLocation sl = connectionFactory.findBestServer(currentServer, excludedServers); + if (sl == null || sl.equals(currentServer)) { + // we didn't find a server to create a replacement cnx on so + // extends the currentServers life + allConnectionsMap.extendLifeOfCnxToServer(currentServer); break; - } else { - if (!this.allConnectionsMap.hasExpiredCnxToServer(currentServer)) { - break; - } - Connection con = null; - try { - con = this.connectionFactory.createClientToServerConnection(sl, false); - } catch (GemFireSecurityException e) { - securityLogWriter.warning( - String.format("Security exception connecting to server '%s': %s", - new Object[] {sl, e})); - } catch (ServerRefusedConnectionException srce) { - logger.warn("Server '{}' refused new connection: {}", - new Object[] {sl, srce}); - } - if (con == null) { - excludedServers.add(sl); - sl = this.connectionFactory.findBestServer(currentServer, excludedServers); - } else { + } + if (!allConnectionsMap.hasExpiredCnxToServer(currentServer)) { + break; + } + Connection con = null; + try { + con = connectionFactory.createClientToServerConnection(sl, false); + if (con != null) { getPoolStats().incLoadConditioningConnect(); - if (!this.allConnectionsMap.hasExpiredCnxToServer(currentServer)) { + if (allConnectionsMap.hasExpiredCnxToServer(currentServer)) { + offerReplacementConnection(con, currentServer); + } else { getPoolStats().incLoadConditioningReplaceTimeouts(); con.destroy(); - break; } - offerReplacementConnection(con, currentServer); break; } + } catch (GemFireSecurityException e) { + securityLogWriter.warning( + String.format("Security exception connecting to server '%s': %s", + new Object[] {sl, e})); + } catch (ServerRefusedConnectionException srce) { + logger.warn("Server '{}' refused new connection: {}", + new Object[] {sl, srce}); } + excludedServers.add(sl); } - if (sl == null) { - // we didn't find a server to create a replacement cnx on so - // extends the currentServers life - this.allConnectionsMap.extendLifeOfCnxToServer(currentServer); - } - return this.allConnectionsMap.checkForReschedule(true); + return allConnectionsMap.checkForReschedule(true); } protected class ConnectionMap { - private final HashMap/* */ map = new HashMap(); - private List/* */ allConnections = new LinkedList/* */(); // in - // the - // order - // they - // were - // created + private final Map> map = new HashMap<>(); + private List allConnections = new LinkedList<>(); private boolean haveLifetimeExpireConnectionsTask; volatile boolean closing; public synchronized boolean isIdleExpirePossible() { - return this.allConnections.size() > minConnections; + return this.allConnections.size() > connectionAccounting.getMinimum(); } @Override @@ -991,25 +820,23 @@ public synchronized void addConnection(PooledConnection connection) { if (this.closing) { throw new CacheClosedException("This pool is closing"); } - synchronized (this) { - getPoolStats().incPoolConnections(1); + getPoolStats().incPoolConnections(1); - // we want the smallest birthDate (e.g. oldest cnx) at the front of the list - this.allConnections.add(connection); + // we want the smallest birthDate (e.g. oldest cnx) at the front of the list + this.allConnections.add(connection); - addToEndpointMap(connection); + addToEndpointMap(connection); - if (isIdleExpirePossible()) { - startBackgroundExpiration(); - } - if (lifetimeTimeout != -1 && !haveLifetimeExpireConnectionsTask) { - if (checkForReschedule(true)) { - // something has already expired so start processing with no delay - startBackgroundLifetimeExpiration(0); - } else { - // either no possible lifetime expires or we scheduled one - } + if (isIdleExpirePossible()) { + startBackgroundExpiration(); + } + if (lifetimeTimeout != -1 && !haveLifetimeExpireConnectionsTask) { + if (checkForReschedule(true)) { + // something has already expired so start processing with no delay + startBackgroundLifetimeExpiration(0); + } else { + // either no possible lifetime expires or we scheduled one } } } @@ -1029,11 +856,11 @@ public synchronized void addReplacedCnx(PooledConnection con, Endpoint oldEndpoi } } - public synchronized Set removeEndpoint(Endpoint endpoint) { - final Set endpointConnections = (Set) this.map.remove(endpoint); + public synchronized Set removeEndpoint(Endpoint endpoint) { + final Set endpointConnections = this.map.remove(endpoint); if (endpointConnections != null) { int count = 0; - for (Iterator it = this.allConnections.iterator(); it.hasNext();) { + for (Iterator it = this.allConnections.iterator(); it.hasNext();) { if (endpointConnections.contains(it.next())) { count++; it.remove(); @@ -1046,10 +873,6 @@ public synchronized Set removeEndpoint(Endpoint endpoint) { return endpointConnections; } - public synchronized boolean containsConnection(PooledConnection connection) { - return this.allConnections.contains(connection); - } - public synchronized boolean removeConnection(PooledConnection connection) { boolean result = this.allConnections.remove(connection); if (result) { @@ -1061,9 +884,9 @@ public synchronized boolean removeConnection(PooledConnection connection) { } private synchronized void addToEndpointMap(PooledConnection connection) { - Set endpointConnections = (Set) map.get(connection.getEndpoint()); + Set endpointConnections = map.get(connection.getEndpoint()); if (endpointConnections == null) { - endpointConnections = new HashSet(); + endpointConnections = new HashSet<>(); map.put(connection.getEndpoint(), endpointConnections); } endpointConnections.add(connection); @@ -1075,7 +898,7 @@ private void removeFromEndpointMap(PooledConnection connection) { private synchronized void removeFromEndpointMap(Endpoint endpoint, PooledConnection connection) { - Set endpointConnections = (Set) this.map.get(endpoint); + Set endpointConnections = this.map.get(endpoint); if (endpointConnections != null) { endpointConnections.remove(connection); if (endpointConnections.size() == 0) { @@ -1136,8 +959,7 @@ public synchronized void emergencyClose() { */ public synchronized PooledConnection findReplacementTarget(ServerLocation currentServer) { final long now = System.nanoTime(); - for (Iterator it = this.allConnections.iterator(); it.hasNext();) { - PooledConnection pc = (PooledConnection) it.next(); + for (PooledConnection pc : allConnections) { if (currentServer.equals(pc.getServer())) { if (!pc.shouldDestroy() && pc.remainingLife(now, lifetimeTimeoutNanos) <= 0) { removeFromEndpointMap(pc); @@ -1155,8 +977,7 @@ public synchronized PooledConnection findReplacementTarget(ServerLocation curren public synchronized boolean hasExpiredCnxToServer(ServerLocation currentServer) { if (!this.allConnections.isEmpty()) { final long now = System.nanoTime(); - for (Iterator it = this.allConnections.iterator(); it.hasNext();) { - PooledConnection pc = (PooledConnection) it.next(); + for (PooledConnection pc : allConnections) { if (pc.shouldDestroy()) { // this con has already been destroyed so ignore it continue; @@ -1180,8 +1001,7 @@ public synchronized boolean hasExpiredCnxToServer(ServerLocation currentServer) public synchronized boolean checkForReschedule(boolean rescheduleOk) { if (!this.allConnections.isEmpty()) { final long now = System.nanoTime(); - for (Iterator it = this.allConnections.iterator(); it.hasNext();) { - PooledConnection pc = (PooledConnection) it.next(); + for (PooledConnection pc : allConnections) { if (pc.hasIdleExpired(now, idleTimeoutNanos)) { // this con has already idle expired so ignore it continue; @@ -1212,8 +1032,8 @@ public synchronized boolean checkForReschedule(boolean rescheduleOk) { public synchronized void extendLifeOfCnxToServer(ServerLocation sl) { if (!this.allConnections.isEmpty()) { final long now = System.nanoTime(); - for (Iterator it = this.allConnections.iterator(); it.hasNext();) { - PooledConnection pc = (PooledConnection) it.next(); + for (Iterator it = this.allConnections.iterator(); it.hasNext();) { + PooledConnection pc = it.next(); if (pc.remainingLife(now, lifetimeTimeoutNanos) > 0) { // no more connections whose lifetime could have expired break; @@ -1247,36 +1067,36 @@ public synchronized void startBackgroundLifetimeExpiration(long delay) { } } - public void checkIdleExpiration() { + public void expireIdleConnections() { int expireCount = 0; List toClose = null; synchronized (this) { haveIdleExpireConnectionsTask = false; - if (shuttingDown) { + if (shuttingDown.get()) { return; } if (logger.isTraceEnabled()) { logger.trace("Looking for connections to expire"); } - // because we expire thread local connections we need to scan allConnections - // find connections which have idle expired - int conCount = this.allConnections.size(); - if (conCount <= minConnections) { + if (!connectionAccounting.isOverMinimum()) { return; } - final long now = System.nanoTime(); + long minRemainingIdle = Long.MAX_VALUE; - toClose = new ArrayList(conCount - minConnections); - for (Iterator it = this.allConnections.iterator(); it.hasNext() - && conCount > minConnections;) { - PooledConnection pc = (PooledConnection) it.next(); + int conCount = connectionAccounting.getCount(); + toClose = new ArrayList<>(conCount - connectionAccounting.getMinimum()); + + // because we expire thread local connections we need to scan allConnections + for (Iterator it = allConnections.iterator(); it.hasNext() + && conCount > connectionAccounting.getMinimum();) { + PooledConnection pc = it.next(); if (pc.shouldDestroy()) { // ignore these connections conCount--; } else { - long remainingIdle = pc.doIdleTimeout(now, idleTimeoutNanos); + long remainingIdle = pc.doIdleTimeout(System.nanoTime(), idleTimeoutNanos); if (remainingIdle >= 0) { if (remainingIdle == 0) { // someone else already destroyed pc so ignore it @@ -1294,7 +1114,8 @@ public void checkIdleExpiration() { } } } - if (conCount > minConnections && minRemainingIdle < Long.MAX_VALUE) { + if (conCount > connectionAccounting.getMinimum() + && minRemainingIdle < Long.MAX_VALUE) { try { backgroundProcessor.schedule(new IdleExpireConnectionsTask(), minRemainingIdle, TimeUnit.NANOSECONDS); @@ -1308,32 +1129,19 @@ public void checkIdleExpiration() { if (expireCount > 0) { getPoolStats().incIdleExpire(expireCount); getPoolStats().incPoolConnections(-expireCount); - // do this outside the above sync - lock.lock(); - try { - connectionCount -= expireCount; - freeConnection.signalAll(); - if (connectionCount < minConnections) { - startBackgroundPrefill(); - } - } finally { - lock.unlock(); - } + destroyAndMaybePrefill(expireCount); } + // now destroy all of the connections, outside the sync - // if (toClose != null) (cannot be null) final boolean isDebugEnabled = logger.isDebugEnabled(); - { - for (Iterator itr = toClose.iterator(); itr.hasNext();) { - PooledConnection connection = (PooledConnection) itr.next(); - if (isDebugEnabled) { - logger.debug("Idle connection detected. Expiring connection {}", connection); - } - try { - connection.internalClose(false); - } catch (Exception e) { - logger.warn("Error expiring connection {}", connection); - } + for (PooledConnection connection : toClose) { + if (isDebugEnabled) { + logger.debug("Idle connection detected. Expiring connection {}", connection); + } + try { + connection.internalClose(false); + } catch (Exception e) { + logger.warn("Error expiring connection {}", connection); } } } @@ -1342,19 +1150,18 @@ public void checkLifetimes() { boolean done; synchronized (this) { this.haveLifetimeExpireConnectionsTask = false; - if (shuttingDown) { + if (shuttingDown.get()) { return; } } do { getPoolStats().incLoadConditioningCheck(); long firstLife = -1; - done = true; ServerLocation candidate = null; boolean idlePossible = true; synchronized (this) { - if (shuttingDown) { + if (shuttingDown.get()) { return; } // find a connection whose lifetime has expired @@ -1362,9 +1169,10 @@ public void checkLifetimes() { long now = System.nanoTime(); long life = 0; idlePossible = isIdleExpirePossible(); - for (Iterator it = this.allConnections.iterator(); it.hasNext() && life <= 0 + for (Iterator it = this.allConnections.iterator(); it.hasNext() + && life <= 0 && (candidate == null);) { - PooledConnection pc = (PooledConnection) it.next(); + PooledConnection pc = it.next(); // skip over idle expired and destroyed life = pc.remainingLife(now, lifetimeTimeoutNanos); if (life <= 0) { @@ -1386,7 +1194,7 @@ public void checkLifetimes() { // reschedule startBackgroundLifetimeExpiration(firstLife); } - done = true; // just to be clear + done = true; } } while (!done); // If a lifetimeExpire task is not scheduled at this point then @@ -1416,9 +1224,9 @@ private void logError(StringId message, Throwable t) { } @Override - public void activate(Connection conn) { + public boolean activate(Connection conn) { assert conn instanceof PooledConnection; - ((PooledConnection) conn).activate(); + return ((PooledConnection) conn).activate(); } @Override diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/PooledConnection.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/PooledConnection.java index d47333bbf711..f0d3491ad6a6 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/PooledConnection.java +++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/PooledConnection.java @@ -69,7 +69,11 @@ public boolean isActive() { } } - public void internalDestroy() { + /** + * @return true if internal connection was destroyed by this call; false if already destroyed + */ + public boolean internalDestroy() { + boolean result = false; this.shouldDestroy.set(true); // probably already set but make sure synchronized (this) { this.active = false; @@ -78,8 +82,10 @@ public void internalDestroy() { if (myCon != null) { myCon.destroy(); connection = null; + result = true; } } + return result; } /** @@ -202,7 +208,10 @@ public synchronized boolean switchConnection(Connection newCon) throws Interrupt return true; } - public void activate() { + /** + * @return true if connection activated, false if could not be activated because it is destroyed + */ + public boolean activate() { synchronized (this) { try { while (this.waitingToSwitch) { @@ -211,14 +220,14 @@ public void activate() { } catch (InterruptedException ex) { Thread.currentThread().interrupt(); } - getConnection(); // it checks if we are destroyed + if (isDestroyed() || shouldDestroy()) { + return false; + } if (active) { throw new InternalGemFireException("Connection already active"); } - if (shouldDestroy()) { - throw new ConnectionDestroyedException(); - } active = true; + return true; } } diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplJUnitTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplJUnitTest.java index 098cf53ee5cf..4b5b74df86b3 100644 --- a/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplJUnitTest.java @@ -589,7 +589,8 @@ public void returnConnection(Connection connection, boolean accessed) { public void start(ScheduledExecutorService backgroundProcessor) {} @Override - public Connection exchangeConnection(Connection conn, Set excludedServers, long aquireTimeout) { + public Connection exchangeConnection(Connection conn, Set excludedServers, + long aquireTimeout) { if (excludedServers.size() >= numServers) { throw new NoAvailableServersException(); } @@ -608,7 +609,9 @@ public Connection getConnection(Connection conn) { } @Override - public void activate(Connection conn) {} + public boolean activate(Connection conn) { + return true; + } @Override public void passivate(Connection conn, boolean accessed) {} diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java new file mode 100644 index 000000000000..1cc1c981b1d6 --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java @@ -0,0 +1,230 @@ +/* + * 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.cache.client.internal.pooling; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.junit.Test; + +public class AvailableConnectionManagerTest { + + private final AvailableConnectionManager instance = new AvailableConnectionManager(); + + private PooledConnection createConnection() { + PooledConnection result = mock(PooledConnection.class); + when(result.activate()).thenReturn(true); + when(result.isActive()).thenReturn(true); + return result; + } + + @Test + public void useFirstReturnsNullGivenEmptyManager() { + instance.getDeque().clear(); + + PooledConnection result = instance.useFirst(); + + assertThat(result).isNull(); + } + + @Test + public void useFirstReturnsExpectedConnectionGivenManagerWithOneItem() { + PooledConnection expected = createConnection(); + instance.getDeque().addFirst(expected); + + PooledConnection result = instance.useFirst(); + + assertThat(result).isSameAs(expected); + assertThat(instance.getDeque()).isEmpty(); + verify(expected).activate(); + } + + @Test + public void useFirstReturnsNullGivenManagerWithOneItemThatCantBeActivated() { + PooledConnection expected = createConnection(); + when(expected.activate()).thenReturn(false); + instance.getDeque().addFirst(expected); + + PooledConnection result = instance.useFirst(); + + assertThat(result).isNull(); + assertThat(instance.getDeque()).isEmpty(); + verify(expected).activate(); + } + + @Test + public void useFirstWithPredicateReturnsNullGivenEmptyManager() { + instance.getDeque().clear(); + + PooledConnection result = instance.useFirst(c -> true); + + assertThat(result).isNull(); + } + + @Test + public void useFirstWithPredicateReturnsExpectedGivenManagerWithOneItem() { + PooledConnection expected = createConnection(); + instance.getDeque().addFirst(expected); + + PooledConnection result = instance.useFirst(c -> c == expected); + + assertThat(result).isSameAs(expected); + assertThat(instance.getDeque()).isEmpty(); + verify(expected).activate(); + } + + @Test + public void useFirstWithPredicateReturnsNullGivenManagerWithOneItemThatDoesNotMatch() { + PooledConnection expected = createConnection(); + instance.getDeque().addFirst(expected); + + PooledConnection result = instance.useFirst(c -> false); + + assertThat(result).isNull(); + assertThat(instance.getDeque()).hasSize(1); + verify(expected, never()).activate(); + } + + @Test + public void useFirstWithPredicateReturnsNullGivenManagerWithOneItemThatCantBeActivated() { + PooledConnection expected = createConnection(); + when(expected.activate()).thenReturn(false); + instance.getDeque().addFirst(expected); + + PooledConnection result = instance.useFirst(c -> c == expected); + + assertThat(result).isNull(); + assertThat(instance.getDeque()).isEmpty(); + verify(expected).activate(); + } + + @Test + public void removeReturnsFalseGivenConnectionNotInManager() { + instance.getDeque().clear(); + + boolean result = instance.remove(createConnection()); + + assertThat(result).isFalse(); + } + + @Test + public void removeReturnsTrueGivenConnectionInManager() { + PooledConnection connection = createConnection(); + instance.getDeque().addFirst(connection); + + boolean result = instance.remove(connection); + + assertThat(result).isTrue(); + } + + @Test + public void removeEmptiesDequeGivenConnectionInManager() { + PooledConnection connection = createConnection(); + instance.getDeque().addFirst(connection); + + instance.remove(connection); + + assertThat(instance.getDeque()).isEmpty(); + } + + @Test + public void addFirstWithTrueAddsActiveConnectionToManager() { + PooledConnection connection = createConnection(); + + instance.addFirst(connection, true); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection).passivate(true); + } + + @Test + public void addFirstWithFalseAddsActiveConnectionToManager() { + PooledConnection connection = createConnection(); + + instance.addFirst(connection, false); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection).passivate(false); + } + + @Test + public void addFirstAddsInactiveConnectionToManager() { + PooledConnection connection = createConnection(); + when(connection.isActive()).thenReturn(false); + + instance.addFirst(connection, true); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection, never()).passivate(anyBoolean()); + } + + + @Test + public void addLastWithTrueAddsActiveConnectionToManager() { + PooledConnection connection = createConnection(); + + instance.addLast(connection, true); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection).passivate(true); + } + + @Test + public void addLastWithFalseAddsActiveConnectionToManager() { + PooledConnection connection = createConnection(); + + instance.addLast(connection, false); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection).passivate(false); + } + + @Test + public void addLastAddsInactiveConnectionToManager() { + PooledConnection connection = createConnection(); + when(connection.isActive()).thenReturn(false); + + instance.addLast(connection, true); + + assertThat(instance.getDeque()).hasSize(1); + verify(connection).isActive(); + verify(connection, never()).passivate(anyBoolean()); + } + + @Test + public void addFirstTakesPrecedenceOverAddLast() { + PooledConnection expected = createConnection(); + + instance.addLast(createConnection(), true); + instance.addFirst(expected, true); + instance.addLast(createConnection(), true); + PooledConnection connection = instance.useFirst(); + + assertThat(instance.getDeque()).hasSize(2); + assertThat(connection).isSameAs(expected); + } + +} diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingTest.java new file mode 100644 index 000000000000..35bf3dccc608 --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/ConnectionAccountingTest.java @@ -0,0 +1,223 @@ +/* + * 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.cache.client.internal.pooling; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.Test; + +public class ConnectionAccountingTest { + @Test + public void constructorSetsMinMax() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + assertThat(a.getMinimum()).isEqualTo(1); + assertThat(a.getMaximum()).isEqualTo(2); + assertThat(a.getCount()).isEqualTo(0); + } + + @Test + public void canPrefillWhenUnderMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + assertThat(a.tryPrefill()).isTrue(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void cantPrefillWhenAtMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + assertThat(a.tryPrefill()).isFalse(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void cantPrefillWhenAboveMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(2); + + assertThat(a.tryPrefill()).isFalse(); + assertThat(a.getCount()).isEqualTo(2); + } + + @Test + public void cancelPrefillDecrements() { + ConnectionAccounting a = new ConnectionAccounting(2, 3); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + assertThat(a.tryPrefill()).isTrue(); + assertThat(a.getCount()).isEqualTo(2); + + a.cancelTryPrefill(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void canCreateWhenUnderMax() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + assertThat(a.getCount()).isEqualTo(0); + + assertThat(a.tryCreate()).isTrue(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void cantCreateWhenAtMax() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + assertThat(a.tryCreate()).isFalse(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void createRegardlessOfMax() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + a.create(); + assertThat(a.getCount()).isEqualTo(2); + assertThat(a.getMaximum()).isEqualTo(1); + } + + @Test + public void cancelCreateDecrementsCount() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.tryCreate(); + assertThat(a.getCount()).isEqualTo(1); + + a.cancelTryCreate(); + assertThat(a.getCount()).isEqualTo(0); + } + + @Test + public void tryDestroyDestroysAConnectionOverMax() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(2); + + assertThat(a.tryDestroy()).isTrue(); + } + + @Test + public void tryDoesNotDestroyAtMax() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + assertThat(a.tryDestroy()).isFalse(); + } + + @Test + public void cancelTryDestroyIncrementsCount() { + ConnectionAccounting a = new ConnectionAccounting(0, 1); + a.create(); + a.create(); + a.tryDestroy(); + assertThat(a.getCount()).isEqualTo(1); + + a.cancelTryDestroy(); + assertThat(a.getCount()).isEqualTo(2); + } + + @Test + public void destroyAndIsUnderMinimumReturnsTrueGoingBelowMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + assertThat(a.getCount()).isEqualTo(1); + + assertThat(a.destroyAndIsUnderMinimum(1)).isTrue(); + assertThat(a.getCount()).isEqualTo(0); + } + + @Test + public void destroyAndIsUnderMinimumReturnsFalseGoingToMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(2); + + assertThat(a.destroyAndIsUnderMinimum(1)).isFalse(); + assertThat(a.getCount()).isEqualTo(1); + } + + @Test + public void destroyAndIsUnderMinimumReturnsFalseStayingAboveMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(3); + + assertThat(a.destroyAndIsUnderMinimum(1)).isFalse(); + assertThat(a.getCount()).isEqualTo(2); + } + + @Test + public void destroyAndIsUnderMinimumDecrementsByMultiple() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(3); + + a.destroyAndIsUnderMinimum(3); + assertThat(a.getCount()).isEqualTo(0); + } + + @Test + public void isUnderMinTrueWhenUnderMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + assertThat(a.isUnderMinimum()).isTrue(); + } + + @Test + public void isUnderMinFalseWhenAtOrOverMin() { + ConnectionAccounting a = new ConnectionAccounting(0, 2); + assertThat(a.isUnderMinimum()).isFalse(); + + a.create(); + assertThat(a.getCount()).isEqualTo(1); + assertThat(a.isUnderMinimum()).isFalse(); + } + + @Test + public void isOverMinFalseWhenUnderOrAtMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + assertThat(a.isOverMinimum()).isFalse(); + + a.create(); + assertThat(a.getCount()).isEqualTo(1); + assertThat(a.isOverMinimum()).isFalse(); + } + + @Test + public void isOverMinTrueWhenOverMin() { + ConnectionAccounting a = new ConnectionAccounting(1, 2); + a.create(); + a.create(); + assertThat(a.getCount()).isEqualTo(2); + + assertThat(a.isOverMinimum()).isTrue(); + } +} From 09a90ba928600b14dfec14aa71aa60d051a96de5 Mon Sep 17 00:00:00 2001 From: Helena Bales Date: Mon, 1 Apr 2019 11:15:05 -0700 Subject: [PATCH 5/8] GEODE-5683: remove unnecessarily gendered language (#3384) Changes a couple of comments to call objects 'it' instead of 'he'. --- .../geode/distributed/internal/ClusterDistributionManager.java | 2 +- .../distributed/internal/locks/GrantorRequestProcessor.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java index 805fa6185a41..9c14fd79bd85 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java @@ -3444,7 +3444,7 @@ public void memberDeparted(InternalDistributedMember theId, boolean crashed, Str if (wasAdmin) { // Pretend we received an AdminConsoleDisconnectMessage from the console that // is no longer in the JavaGroup view. - // He must have died without sending a ShutdownMessage. + // It must have died without sending a ShutdownMessage. // This fixes bug 28454. AdminConsoleDisconnectMessage message = new AdminConsoleDisconnectMessage(); message.setSender(theId); diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/GrantorRequestProcessor.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/GrantorRequestProcessor.java index 558f40e04ad0..cc8dbee62a28 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/GrantorRequestProcessor.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/locks/GrantorRequestProcessor.java @@ -394,7 +394,7 @@ private static GrantorInfo basicOp(long grantorVersion, String serviceName, DLoc if (opCode != CLEAR_OP && opCode != CLEAR_WITH_LOCKS_OP) { // Note we do not try a new elder if doing a clear because // the new elder will not have anything for us to clear. - // He will have done an ElderInit. + // It will have done an ElderInit. tryNewElder = true; } } From ed8b17497b091e7eba55f45ae5a25632a7703389 Mon Sep 17 00:00:00 2001 From: Helena Bales Date: Mon, 1 Apr 2019 12:22:06 -0700 Subject: [PATCH 6/8] GEODE-6577: performance gain by removing lazy init (#3378) There was lock contention for checking if the clientMetadataService needed to be initialized. The object is not expensive to construct, so the initialization has been moved the GemFireCacheImpl's constructor, allowing the lock object to be removed completely. --- .../geode/internal/cache/GemFireCacheImpl.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java index 79da8f0ac44c..63031ee2bf2e 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java @@ -550,9 +550,7 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache, Has private final PersistentMemberManager persistentMemberManager; - private ClientMetadataService clientMetadataService = null; - - private final Object clientMetaDatServiceLock = new Object(); + private final ClientMetadataService clientMetadataService; private final AtomicBoolean isShutDownAll = new AtomicBoolean(); private final CountDownLatch shutDownAllFinished = new CountDownLatch(1); @@ -825,6 +823,7 @@ public static GemFireCacheImpl getForPdx(String reason) { .info("Running in local mode since no locators were specified."); } } + } else { logger.info("Running in client mode"); this.resourceEventsListener = null; @@ -936,6 +935,8 @@ public static GemFireCacheImpl getForPdx(String reason) { } } } // synchronized + + clientMetadataService = new ClientMetadataService(this); } @Override @@ -2022,13 +2023,9 @@ public PersistentMemberManager getPersistentMemberManager() { @Override public ClientMetadataService getClientMetadataService() { - synchronized (this.clientMetaDatServiceLock) { - this.stopper.checkCancelInProgress(null); - if (this.clientMetadataService == null) { - this.clientMetadataService = new ClientMetadataService(this); - } - return this.clientMetadataService; - } + this.stopper.checkCancelInProgress(null); + + return this.clientMetadataService; } private final boolean DISABLE_DISCONNECT_DS_ON_CACHE_CLOSE = Boolean From 26c2e5f0866880ea1030d92ad534821995488514 Mon Sep 17 00:00:00 2001 From: Jacob Barrett Date: Mon, 1 Apr 2019 14:09:19 -0700 Subject: [PATCH 7/8] Update Gradle plugins to latest versions. (#3382) Move from deprecated gradle-git to grgit-gradle. Update nebula plugins that were stuck due to deprecated gradle-git. --- build.gradle | 20 ++++++++------------ gradle/rat.gradle | 4 +--- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/build.gradle b/build.gradle index 39c6a0ac89eb..605b1ded5448 100755 --- a/build.gradle +++ b/build.gradle @@ -23,19 +23,15 @@ buildscript { } dependencies { - // Newer versions of the nebula plugins require gradle-info-plugin 4.0.2 or higher, which in - // depends on JGit 5.0. Conversely, gradle-git relies of JGit 4.0. - // Be mindful of potential classpath issues when updating any of these three dependencies. - classpath "com.netflix.nebula:nebula-project-plugin:5.1.0" - classpath "com.netflix.nebula:gradle-lint-plugin:10.1.0" - classpath "org.ajoberstar:gradle-git:1.7.2" - - classpath "gradle.plugin.org.nosphere.apache:creadur-rat-gradle:0.3.1" - classpath 'org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:2.6.2' - classpath "com.diffplug.spotless:spotless-plugin-gradle:3.10.0" + classpath "com.netflix.nebula:nebula-project-plugin:6.0.2" + classpath "com.netflix.nebula:gradle-lint-plugin:11.4.4" + classpath "org.ajoberstar.grgit:grgit-gradle:3.1.1" + classpath "org.nosphere.apache:creadur-rat-gradle:0.4.0" + classpath 'org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:2.7' + classpath "com.diffplug.spotless:spotless-plugin-gradle:3.21.1" classpath "me.champeau.gradle:jmh-gradle-plugin:0.4.8" - classpath 'com.github.ben-manes:gradle-versions-plugin:0.17.0' - classpath 'io.spring.gradle:dependency-management-plugin:1.0.6.RELEASE' + classpath 'com.github.ben-manes:gradle-versions-plugin:0.21.0' + classpath 'io.spring.gradle:dependency-management-plugin:1.0.7.RELEASE' } } diff --git a/gradle/rat.gradle b/gradle/rat.gradle index 63f8db222426..29014cc400d3 100644 --- a/gradle/rat.gradle +++ b/gradle/rat.gradle @@ -14,13 +14,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + apply plugin: "org.nosphere.apache.rat" rat { inputDir = rootDir - xmlOutput = false - htmlOutput = false - plainOutput = true excludes = [ // git From 1ff5f2efadf63f5b220e56d6d8a8a6b3d503b263 Mon Sep 17 00:00:00 2001 From: jinmeiliao Date: Tue, 2 Apr 2019 08:36:14 -0700 Subject: [PATCH 8/8] GEODE-6574: be able to list member information using cluster management service (#3380) Co-authored-by: Jens Deppe * create a MemberConfig object to hold all the essential member information. * added list(CacheElement elem) interface to cms. * added a result object in ClusterManagementResult * excluded jackson libraries in geode-web-management module so that jackson serialization would honor the annotations. --- .../MemberManagementServiceDunitTest.java | 88 ++++++++++++ .../resources/assembly_content.txt | 8 +- geode-common/build.gradle | 1 + .../geode/util/internal/GeodeJsonMapper.java | 37 +++++ .../src/test/resources/expected-pom.xml | 7 + .../cli/converters/PoolPropertyConverter.java | 7 +- ...erManagementServiceRetrievalDUnitTest.java | 2 +- .../RegionConfigMutatorIntegrationTest.java | 4 +- .../distributed/internal/InternalLocator.java | 2 +- .../api/LocatorClusterManagementService.java | 44 ++++-- .../converters/ConfigPropertyConverter.java | 7 +- .../cli/functions/DataCommandFunction.java | 5 +- .../GetMemberInformationFunction.java | 9 +- ...Mutator.java => ConfigurationManager.java} | 6 +- .../mutators/MemberConfigManager.java | 116 +++++++++++++++ ...gMutator.java => RegionConfigManager.java} | 15 +- .../cache/configuration/RegionConfigTest.java | 5 +- .../LocatorClusterManagementServiceTest.java | 10 +- geode-management/build.gradle | 1 + .../cache/configuration/CacheElement.java | 3 + .../cache/configuration/RegionConfig.java | 8 +- .../api/ClusterManagementResult.java | 14 ++ .../api/ClusterManagementService.java | 4 +- .../geode/management/api/RestfulEndpoint.java | 3 + .../configuration/MemberConfig.java | 95 +++++++++++++ .../ClientClusterManagementService.java | 32 ++++- .../internal/cli/domain/ClassName.java | 9 +- .../CacheElementJsonMappingTest.java | 88 ++++++++++++ geode-web-management/build.gradle | 27 ++-- ...ientClusterManagementServiceDUnitTest.java | 13 +- .../MemberManagementServiceDUnitTest.java | 133 ++++++++++++++++++ .../MemberManagementController.java | 67 +++++++++ .../controllers/PingManagementController.java | 35 +++++ .../RegionManagementController.java | 8 +- .../WEB-INF/geode-management-servlet.xml | 1 + .../src/test/resources/expected-pom.xml | 24 +--- 36 files changed, 831 insertions(+), 107 deletions(-) create mode 100644 geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceDunitTest.java create mode 100644 geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java rename geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/{ConfigurationMutator.java => ConfigurationManager.java} (91%) create mode 100644 geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/MemberConfigManager.java rename geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/{RegionConfigMutator.java => RegionConfigManager.java} (74%) create mode 100644 geode-management/src/main/java/org/apache/geode/management/configuration/MemberConfig.java create mode 100644 geode-management/src/test/java/org/apache/geode/cache/configuration/CacheElementJsonMappingTest.java create mode 100644 geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceDUnitTest.java create mode 100644 geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/MemberManagementController.java create mode 100644 geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/PingManagementController.java diff --git a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceDunitTest.java b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceDunitTest.java new file mode 100644 index 000000000000..d6a9f346cff1 --- /dev/null +++ b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/MemberManagementServiceDunitTest.java @@ -0,0 +1,88 @@ +/* + * 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.rest; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import org.apache.geode.cache.configuration.CacheElement; +import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.client.ClusterManagementServiceProvider; +import org.apache.geode.management.configuration.MemberConfig; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; + +public class MemberManagementServiceDunitTest { + @ClassRule + public static ClusterStartupRule cluster = new ClusterStartupRule(2); + + private static MemberVM locator, server; + private static ClusterManagementService cmsClient; + + @BeforeClass + public static void beforeClass() { + locator = cluster.startLocatorVM(0, l -> l.withHttpService()); + server = cluster.startServerVM(1, locator.getPort()); + cmsClient = ClusterManagementServiceProvider.getService("localhost", locator.getHttpPort()); + } + + @Test + public void listAllMembers() { + MemberConfig config = new MemberConfig(); + ClusterManagementResult result = cmsClient.list(config); + + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(2); + + MemberConfig memberConfig = + (MemberConfig) CacheElement.findElement(result.getResult(), "locator-0"); + assertThat(memberConfig.isCoordinator()).isTrue(); + assertThat(memberConfig.isLocator()).isTrue(); + assertThat(memberConfig.getPorts().get(0)).isEqualTo(locator.getPort()); + } + + @Test + public void listOneMember() throws Exception { + MemberConfig config = new MemberConfig(); + config.setId("locator-0"); + + ClusterManagementResult result = cmsClient.list(config); + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(1); + + MemberConfig memberConfig = (MemberConfig) result.getResult().get(0); + assertThat(memberConfig.isCoordinator()).isTrue(); + assertThat(memberConfig.isLocator()).isTrue(); + assertThat(memberConfig.getPorts().get(0)).isEqualTo(locator.getPort()); + } + + @Test + public void listNonExistentMember() throws Exception { + MemberConfig config = new MemberConfig(); + config.setId("locator"); + ClusterManagementResult result = cmsClient.list(config); + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()) + .isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(0); + } +} diff --git a/geode-assembly/src/integrationTest/resources/assembly_content.txt b/geode-assembly/src/integrationTest/resources/assembly_content.txt index 4b098d44d488..9b8a35f8015b 100644 --- a/geode-assembly/src/integrationTest/resources/assembly_content.txt +++ b/geode-assembly/src/integrationTest/resources/assembly_content.txt @@ -705,6 +705,10 @@ javadoc/org/apache/geode/management/client/ClusterManagementServiceProvider.html javadoc/org/apache/geode/management/client/package-frame.html javadoc/org/apache/geode/management/client/package-summary.html javadoc/org/apache/geode/management/client/package-tree.html +javadoc/org/apache/geode/management/configuration/MemberConfig.html +javadoc/org/apache/geode/management/configuration/package-frame.html +javadoc/org/apache/geode/management/configuration/package-summary.html +javadoc/org/apache/geode/management/configuration/package-tree.html javadoc/org/apache/geode/management/membership/ClientMembership.html javadoc/org/apache/geode/management/membership/ClientMembershipEvent.html javadoc/org/apache/geode/management/membership/ClientMembershipListener.html @@ -904,13 +908,13 @@ lib/geode-cq-0.0.0.jar lib/geode-dependencies.jar lib/geode-jca-0.0.0.rar lib/geode-lucene-0.0.0.jar -lib/geode-redis-0.0.0.jar -lib/geode-memcached-0.0.0.jar lib/geode-management-0.0.0.jar +lib/geode-memcached-0.0.0.jar lib/geode-old-client-support-0.0.0.jar lib/geode-protobuf-0.0.0.jar lib/geode-protobuf-messages-0.0.0.jar lib/geode-rebalancer-0.0.0.jar +lib/geode-redis-0.0.0.jar lib/geode-wan-0.0.0.jar lib/gfsh-dependencies.jar lib/grumpy-core-0.2.2.jar diff --git a/geode-common/build.gradle b/geode-common/build.gradle index 6a2ed39fd37a..9bf17b16d1a8 100755 --- a/geode-common/build.gradle +++ b/geode-common/build.gradle @@ -19,6 +19,7 @@ apply from: "${project.projectDir}/../gradle/publish.gradle" dependencies { compile(platform(project(':boms:geode-all-bom'))) + compile('com.fasterxml.jackson.core:jackson-databind') testCompile('junit:junit') testCompile('org.assertj:assertj-core') } diff --git a/geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java b/geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java new file mode 100644 index 000000000000..7fd8f916d097 --- /dev/null +++ b/geode-common/src/main/java/org/apache/geode/util/internal/GeodeJsonMapper.java @@ -0,0 +1,37 @@ +/* + * 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.util.internal; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.MapperFeature; +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * helper class for creating various json mappers used by Geode Project + */ +public class GeodeJsonMapper { + + /** + * @return a jackson json mapper that allows single quotes and is able to deserialize json + * string without @JsonTypeInfo if base class is a concrete implementation. + */ + public static ObjectMapper getMapper() { + ObjectMapper mapper = new ObjectMapper(); + mapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true); + mapper.configure(MapperFeature.USE_BASE_TYPE_AS_DEFAULT_IMPL, true); + return mapper; + } +} diff --git a/geode-common/src/test/resources/expected-pom.xml b/geode-common/src/test/resources/expected-pom.xml index 001211712862..259eb6621fc4 100644 --- a/geode-common/src/test/resources/expected-pom.xml +++ b/geode-common/src/test/resources/expected-pom.xml @@ -45,4 +45,11 @@ + + + com.fasterxml.jackson.core + jackson-databind + compile + + diff --git a/geode-connectors/src/main/java/org/apache/geode/management/internal/cli/converters/PoolPropertyConverter.java b/geode-connectors/src/main/java/org/apache/geode/management/internal/cli/converters/PoolPropertyConverter.java index d99709cc40fa..6127a1856e5e 100644 --- a/geode-connectors/src/main/java/org/apache/geode/management/internal/cli/converters/PoolPropertyConverter.java +++ b/geode-connectors/src/main/java/org/apache/geode/management/internal/cli/converters/PoolPropertyConverter.java @@ -17,13 +17,13 @@ import java.io.IOException; import java.util.List; -import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.ObjectMapper; import org.springframework.shell.core.Completion; import org.springframework.shell.core.Converter; import org.springframework.shell.core.MethodTarget; import org.apache.geode.connectors.jdbc.internal.cli.CreateDataSourceCommand; +import org.apache.geode.util.internal.GeodeJsonMapper; /*** * Converter for CreateDataSourceCommand's --pool-properties option. @@ -32,10 +32,7 @@ public class PoolPropertyConverter implements Converter { - private static final ObjectMapper mapper = new ObjectMapper(); - static { - mapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true); - } + private static final ObjectMapper mapper = GeodeJsonMapper.getMapper(); @Override public boolean supports(Class type, String optionContext) { diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/api/ClusterManagementServiceRetrievalDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/management/api/ClusterManagementServiceRetrievalDUnitTest.java index f34f8573e1d3..2deaabca2395 100644 --- a/geode-core/src/distributedTest/java/org/apache/geode/management/api/ClusterManagementServiceRetrievalDUnitTest.java +++ b/geode-core/src/distributedTest/java/org/apache/geode/management/api/ClusterManagementServiceRetrievalDUnitTest.java @@ -41,7 +41,7 @@ public void retrieveClusterManagementServiceFromServer() { server1 = cluster.startServerVM(1, locator.getPort()); final String url = - String.format("http://localhost:%d/geode-management/v2", locator.getHttpPort()); + String.format("http://localhost:%d/geode-management", locator.getHttpPort()); server1.invoke(() -> { ClientClusterManagementService cms = (ClientClusterManagementService) ClusterManagementServiceProvider.getService(); diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutatorIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutatorIntegrationTest.java index 5a599bb8783c..d7068a3892fa 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutatorIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutatorIntegrationTest.java @@ -31,13 +31,13 @@ public class RegionConfigMutatorIntegrationTest { @Rule public LocatorStarterRule locator = new LocatorStarterRule().withAutoStart(); - private RegionConfigMutator mutator; + private RegionConfigManager mutator; private RegionConfig config; @Before public void before() throws Exception { config = new RegionConfig(); - mutator = new RegionConfigMutator(); + mutator = new RegionConfigManager(); } @Test diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java index 46b80d5014a7..127a26466cee 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java @@ -691,7 +691,7 @@ private void startClusterManagementService() { } clusterManagementService = - new LocatorClusterManagementService(locator.myCache.getDistributionManager(), + new LocatorClusterManagementService(locator.myCache, locator.configurationPersistenceService); // start management rest service 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 d9abc0243a46..511cc3f67a9c 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 @@ -33,42 +33,45 @@ import org.apache.geode.cache.execute.ResultCollector; import org.apache.geode.distributed.ConfigurationPersistenceService; import org.apache.geode.distributed.DistributedMember; -import org.apache.geode.distributed.internal.DistributionManager; +import org.apache.geode.internal.cache.InternalCache; import org.apache.geode.internal.logging.LogService; import org.apache.geode.management.api.ClusterManagementResult; import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.configuration.MemberConfig; import org.apache.geode.management.internal.cli.CliUtil; import org.apache.geode.management.internal.cli.functions.CliFunctionResult; import org.apache.geode.management.internal.cli.functions.UpdateCacheFunction; -import org.apache.geode.management.internal.configuration.mutators.ConfigurationMutator; -import org.apache.geode.management.internal.configuration.mutators.RegionConfigMutator; +import org.apache.geode.management.internal.configuration.mutators.ConfigurationManager; +import org.apache.geode.management.internal.configuration.mutators.MemberConfigManager; +import org.apache.geode.management.internal.configuration.mutators.RegionConfigManager; import org.apache.geode.management.internal.configuration.validators.ConfigurationValidator; import org.apache.geode.management.internal.configuration.validators.RegionConfigValidator; import org.apache.geode.management.internal.exceptions.EntityExistsException; public class LocatorClusterManagementService implements ClusterManagementService { private static final Logger logger = LogService.getLogger(); - private DistributionManager distributionManager; + private InternalCache cache; private ConfigurationPersistenceService persistenceService; - private HashMap mutators; + private HashMap managers; private HashMap validators; - public LocatorClusterManagementService(DistributionManager distributionManager, + public LocatorClusterManagementService(InternalCache cache, ConfigurationPersistenceService persistenceService) { - this(distributionManager, persistenceService, new HashMap(), new HashMap()); - // initialize the list of mutators - mutators.put(RegionConfig.class, new RegionConfigMutator()); + this(cache, persistenceService, new HashMap(), new HashMap()); + // initialize the list of managers + managers.put(RegionConfig.class, new RegionConfigManager()); + managers.put(MemberConfig.class, new MemberConfigManager(cache)); // initialize the list of validators validators.put(RegionConfig.class, new RegionConfigValidator()); } @VisibleForTesting - public LocatorClusterManagementService(DistributionManager distributionManager, - ConfigurationPersistenceService persistenceService, HashMap mutators, HashMap validators) { - this.distributionManager = distributionManager; + public LocatorClusterManagementService(InternalCache cache, + ConfigurationPersistenceService persistenceService, HashMap managers, HashMap validators) { + this.cache = cache; this.persistenceService = persistenceService; - this.mutators = mutators; + this.managers = managers; this.validators = validators; } @@ -84,7 +87,7 @@ public ClusterManagementResult create(CacheElement config, String group) { } ClusterManagementResult result = new ClusterManagementResult(); - ConfigurationMutator configurationMutator = mutators.get(config.getClass()); + ConfigurationManager configurationMutator = managers.get(config.getClass()); ConfigurationValidator validator = validators.get(config.getClass()); if (validator != null) { @@ -147,6 +150,17 @@ public ClusterManagementResult update(CacheElement config, String group) { throw new NotImplementedException("Not implemented"); } + @Override + public ClusterManagementResult list(CacheElement filter) { + ConfigurationManager manager = managers.get(filter.getClass()); + List listResults = manager.list(filter, null); + + ClusterManagementResult result = new ClusterManagementResult(); + result.setResult(listResults); + + return result; + } + @Override public boolean isConnected() { return true; @@ -154,7 +168,7 @@ public boolean isConnected() { @VisibleForTesting Set findMembers(String[] groups, String[] members) { - return CliUtil.findMembers(groups, members, distributionManager); + return CliUtil.findMembers(groups, members, cache); } @VisibleForTesting diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ConfigPropertyConverter.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ConfigPropertyConverter.java index 8a17f3cd2401..adff1eac6658 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ConfigPropertyConverter.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ConfigPropertyConverter.java @@ -17,7 +17,6 @@ import java.io.IOException; import java.util.List; -import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.ObjectMapper; import org.springframework.shell.core.Completion; import org.springframework.shell.core.Converter; @@ -25,6 +24,7 @@ import org.apache.geode.annotations.Immutable; import org.apache.geode.cache.configuration.JndiBindingsType; +import org.apache.geode.util.internal.GeodeJsonMapper; /*** * Added converter to enable auto-completion for index-type @@ -34,10 +34,7 @@ public class ConfigPropertyConverter implements Converter { @Immutable - private static final ObjectMapper mapper = new ObjectMapper(); - static { - mapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true); - } + private static final ObjectMapper mapper = GeodeJsonMapper.getMapper(); @Override public boolean supports(Class type, String optionContext) { diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java index 3da2b6a85542..95c4caa09a5f 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DataCommandFunction.java @@ -22,7 +22,6 @@ import java.util.Map; import java.util.Set; -import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.lang3.StringUtils; @@ -59,6 +58,7 @@ import org.apache.geode.management.internal.cli.util.JsonUtil; import org.apache.geode.pdx.JSONFormatter; import org.apache.geode.pdx.PdxInstance; +import org.apache.geode.util.internal.GeodeJsonMapper; /** * @since GemFire 7.0 @@ -586,8 +586,7 @@ private Object getClassObject(String string, String klassString) Object resultObject; try { - ObjectMapper mapper = new ObjectMapper(); - mapper.enable(JsonParser.Feature.ALLOW_SINGLE_QUOTES); + ObjectMapper mapper = GeodeJsonMapper.getMapper(); resultObject = mapper.readValue(string, klass); } catch (IOException e) { throw new IllegalArgumentException( diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GetMemberInformationFunction.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GetMemberInformationFunction.java index f0c3562061c8..18457ad9025f 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GetMemberInformationFunction.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/GetMemberInformationFunction.java @@ -30,6 +30,7 @@ import org.apache.geode.distributed.DistributedMember; import org.apache.geode.distributed.internal.DistributionConfig; import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.distributed.internal.InternalLocator; import org.apache.geode.internal.admin.SSLConfig; import org.apache.geode.internal.cache.CacheClientStatus; import org.apache.geode.internal.cache.InternalCache; @@ -118,7 +119,7 @@ public void execute(FunctionContext functionContext) { List csList = cache.getCacheServers(); // A member is a server only if it has a cacheserver - if (csList != null) { + if (csList != null && !csList.isEmpty()) { memberInfo.setServer(true); Iterator iters = csList.iterator(); while (iters.hasNext()) { @@ -143,6 +144,10 @@ public void execute(FunctionContext functionContext) { memberInfo.setClientCount(numConnections); } else { memberInfo.setServer(false); + InternalLocator locator = InternalLocator.getLocator(); + if (locator != null) { + memberInfo.setLocatorPort(locator.getPort()); + } } functionContext.getResultSender().lastResult(memberInfo); } catch (CacheClosedException e) { @@ -152,6 +157,8 @@ public void execute(FunctionContext functionContext) { } } + + private long bytesToMeg(long bytes) { return bytes / (1024L * 1024L); } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationMutator.java b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationManager.java similarity index 91% rename from geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationMutator.java rename to geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationManager.java index 5f68d4577d79..c3c11054cdac 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationMutator.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/ConfigurationManager.java @@ -17,6 +17,8 @@ package org.apache.geode.management.internal.configuration.mutators; +import java.util.List; + import org.apache.geode.annotations.Experimental; import org.apache.geode.cache.configuration.CacheConfig; import org.apache.geode.cache.configuration.CacheElement; @@ -28,10 +30,12 @@ * type {@link CacheElement}, which represents the configuration change. */ @Experimental -public interface ConfigurationMutator { +public interface ConfigurationManager { void add(T config, CacheConfig existing); void update(T config, CacheConfig existing); void delete(T config, CacheConfig existing); + + List list(T filterConfig, CacheConfig existing); } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/MemberConfigManager.java b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/MemberConfigManager.java new file mode 100644 index 000000000000..3cbf5337c302 --- /dev/null +++ b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/MemberConfigManager.java @@ -0,0 +1,116 @@ +/* + * 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.configuration.mutators; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.commons.lang3.NotImplementedException; + +import org.apache.geode.cache.configuration.CacheConfig; +import org.apache.geode.cache.execute.Execution; +import org.apache.geode.cache.execute.FunctionService; +import org.apache.geode.cache.execute.ResultCollector; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.distributed.internal.membership.MembershipManager; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.management.configuration.MemberConfig; +import org.apache.geode.management.internal.cli.domain.CacheServerInfo; +import org.apache.geode.management.internal.cli.domain.MemberInformation; +import org.apache.geode.management.internal.cli.functions.GetMemberInformationFunction; + +public class MemberConfigManager implements ConfigurationManager { + + private InternalCache cache; + + public MemberConfigManager(InternalCache cache) { + this.cache = cache; + } + + @Override + public void add(MemberConfig config, CacheConfig existing) { + throw new NotImplementedException("Not implemented"); + } + + @Override + public void update(MemberConfig config, CacheConfig existing) { + throw new NotImplementedException("Not implemented"); + } + + @Override + public void delete(MemberConfig config, CacheConfig existing) { + throw new NotImplementedException("Not implemented"); + } + + @Override + public List list(MemberConfig filter, CacheConfig existing) { + String coordinatorId = null; + List results = new ArrayList<>(); + + Set members = cache.getDistributionManager().getDistributionManagerIds() + .stream().filter(m -> (filter.getId() == null || filter.getId().equals(m.getName()))) + .map(DistributedMember.class::cast).collect(Collectors.toSet()); + + if (members.size() == 0) { + return results; + } + + for (DistributedMember member : members) { + if (member == getCoordinator()) { + coordinatorId = member.getId(); + } + } + + Execution execution = FunctionService.onMembers(members); + ResultCollector rc = execution.execute(new GetMemberInformationFunction()); + + ArrayList output = (ArrayList) rc.getResult(); + + + for (MemberInformation mInfo : output) { + MemberConfig member = new MemberConfig(); + member.setId(mInfo.getName()); + member.setHost(mInfo.getHost()); + member.setPid(mInfo.getProcessId()); + + if (mInfo.isServer() && mInfo.getCacheServeInfo() != null) { + member.setPorts(mInfo.getCacheServeInfo().stream().map(CacheServerInfo::getPort) + .collect(Collectors.toList())); + member.setLocator(false); + } else { + member.setPorts(Arrays.asList(mInfo.getLocatorPort())); + member.setLocator(true); + } + + member.setCoordinator(mInfo.getId().equals(coordinatorId)); + results.add(member); + } + + return results; + } + + private DistributedMember getCoordinator() { + MembershipManager mmgr = cache.getDistributionManager().getMembershipManager(); + if (mmgr == null) { + return null; + } + + return mmgr.getCoordinator(); + } +} diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutator.java b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigManager.java similarity index 74% rename from geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutator.java rename to geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigManager.java index c96416410117..cc34220101f9 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutator.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigManager.java @@ -17,12 +17,16 @@ package org.apache.geode.management.internal.configuration.mutators; +import java.util.List; + +import org.apache.commons.lang3.NotImplementedException; + import org.apache.geode.cache.configuration.CacheConfig; import org.apache.geode.cache.configuration.RegionConfig; -public class RegionConfigMutator implements ConfigurationMutator { +public class RegionConfigManager implements ConfigurationManager { - public RegionConfigMutator() {} + public RegionConfigManager() {} @Override public void add(RegionConfig configElement, CacheConfig existingConfig) { @@ -31,11 +35,16 @@ public void add(RegionConfig configElement, CacheConfig existingConfig) { @Override public void update(RegionConfig config, CacheConfig existing) { - + throw new NotImplementedException("Not implemented yet"); } @Override public void delete(RegionConfig config, CacheConfig existing) { + throw new NotImplementedException("Not implemented yet"); + } + @Override + public List list(RegionConfig config, CacheConfig existing) { + throw new NotImplementedException("Not implemented yet"); } } diff --git a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java index fa7e298c2d3d..62dd8c352bd9 100644 --- a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java +++ b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java @@ -27,6 +27,7 @@ import org.apache.geode.cache.RegionShortcut; import org.apache.geode.internal.config.JAXBService; +import org.apache.geode.util.internal.GeodeJsonMapper; public class RegionConfigTest { @@ -76,14 +77,14 @@ public void checkDefaultRegionAttributesForShortcuts() { @Test public void correctJsonAndXml() throws Exception { String json = "{\"name\":\"test\", \"type\":\"REPLICATE\"}"; - ObjectMapper mapper = new ObjectMapper(); + ObjectMapper mapper = GeodeJsonMapper.getMapper(); regionConfig = mapper.readValue(json, RegionConfig.class); assertThat(regionConfig.getName()).isEqualTo("test"); assertThat(regionConfig.getType()).isEqualTo("REPLICATE"); String json2 = mapper.writeValueAsString(regionConfig); assertThat(json2).contains("\"type\":\"REPLICATE\""); - assertThat(json2).contains("\"id\":\"test\""); + assertThat(json2).contains("\"name\":\"test\""); CacheConfig cacheConfig = new CacheConfig(); cacheConfig.getRegions().add(regionConfig); diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java index 5b8755d681f3..2aaf71eb4d20 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java @@ -34,7 +34,7 @@ import org.apache.geode.cache.configuration.RegionConfig; import org.apache.geode.distributed.ConfigurationPersistenceService; import org.apache.geode.distributed.DistributedMember; -import org.apache.geode.distributed.internal.DistributionManager; +import org.apache.geode.internal.cache.InternalCache; import org.apache.geode.management.api.ClusterManagementResult; import org.apache.geode.management.internal.cli.functions.CliFunctionResult; import org.apache.geode.management.internal.exceptions.EntityExistsException; @@ -42,22 +42,22 @@ public class LocatorClusterManagementServiceTest { private LocatorClusterManagementService service; - private DistributionManager distributionManager; + private InternalCache cache; private ConfigurationPersistenceService persistenceService; private RegionConfig regionConfig; private ClusterManagementResult result; @Before public void before() throws Exception { - distributionManager = mock(DistributionManager.class); + cache = mock(InternalCache.class); persistenceService = mock(ConfigurationPersistenceService.class); - service = spy(new LocatorClusterManagementService(distributionManager, persistenceService)); + service = spy(new LocatorClusterManagementService(cache, persistenceService)); regionConfig = new RegionConfig(); } @Test public void persistenceIsNull() throws Exception { - service = new LocatorClusterManagementService(distributionManager, null); + service = new LocatorClusterManagementService(cache, null); result = service.create(regionConfig, "cluster"); assertThat(result.isSuccessful()).isFalse(); assertThat(result.getStatusMessage()) diff --git a/geode-management/build.gradle b/geode-management/build.gradle index a30d1df47bca..fede2f32a53c 100755 --- a/geode-management/build.gradle +++ b/geode-management/build.gradle @@ -32,6 +32,7 @@ dependencies { exclude module: 'junit' } + testCompile(project(':geode-common')) testCompile(project(':geode-junit')) { exclude module: 'geode-core' } diff --git a/geode-management/src/main/java/org/apache/geode/cache/configuration/CacheElement.java b/geode-management/src/main/java/org/apache/geode/cache/configuration/CacheElement.java index 070a4d233a06..6ab7173ee82e 100644 --- a/geode-management/src/main/java/org/apache/geode/cache/configuration/CacheElement.java +++ b/geode-management/src/main/java/org/apache/geode/cache/configuration/CacheElement.java @@ -20,10 +20,13 @@ import java.io.Serializable; import java.util.List; +import com.fasterxml.jackson.annotation.JsonTypeInfo; + import org.apache.geode.annotations.Experimental; import org.apache.geode.lang.Identifiable; @Experimental +@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "class") public interface CacheElement extends Identifiable, Serializable { static boolean exists(List list, String id) { diff --git a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java index fb6e831bc6b0..43ea7187e4f3 100644 --- a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java +++ b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java @@ -29,6 +29,8 @@ import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlType; +import com.fasterxml.jackson.annotation.JsonIgnore; + import org.apache.geode.annotations.Experimental; import org.apache.geode.cache.RegionShortcut; import org.apache.geode.management.api.RestfulEndpoint; @@ -152,6 +154,9 @@ propOrder = {"regionAttributes", "indexes", "entries", "regionElements", "regions"}) @Experimental public class RegionConfig implements CacheElement, RestfulEndpoint { + + public static final String REGION_CONFIG_ENDPOINT = "/regions"; + @XmlElement(name = "region-attributes", namespace = "http://geode.apache.org/schema/cache") protected RegionAttributesType regionAttributes; @XmlElement(name = "index", namespace = "http://geode.apache.org/schema/cache") @@ -176,7 +181,7 @@ public RegionConfig(String name, String refid) { @Override public String getEndpoint() { - return "/regions"; + return REGION_CONFIG_ENDPOINT; } public RegionAttributesType getRegionAttributes() { @@ -504,6 +509,7 @@ private void setShortcutAttributes() { } @Override + @JsonIgnore public String getId() { return getName(); } diff --git a/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementResult.java b/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementResult.java index 92e18a1bdf33..2f61e8d55c9c 100644 --- a/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementResult.java +++ b/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementResult.java @@ -14,11 +14,15 @@ */ package org.apache.geode.management.api; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import com.fasterxml.jackson.annotation.JsonIgnore; +import org.apache.geode.cache.configuration.CacheElement; + public class ClusterManagementResult { // this error code should include a one-to-one mapping to the http status code returned @@ -54,6 +58,8 @@ public enum StatusCode { private StatusCode statusCode = StatusCode.OK; private String statusMessage; + private List result = new ArrayList<>(); + public ClusterManagementResult() {} public ClusterManagementResult(boolean success, String message) { @@ -101,4 +107,12 @@ public boolean isSuccessful() { public StatusCode getStatusCode() { return statusCode; } + + public List getResult() { + return result; + } + + public void setResult(List result) { + this.result = result; + } } diff --git a/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementService.java b/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementService.java index d6f8ffa70731..053fd62af172 100644 --- a/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementService.java +++ b/geode-management/src/main/java/org/apache/geode/management/api/ClusterManagementService.java @@ -74,8 +74,10 @@ default ClusterManagementResult update(CacheElement config) { return update(config, null); } + ClusterManagementResult list(CacheElement config); + /** - * Test to see if this instance of ClsuterManagmentService retrieved from the client side is + * Test to see if this instance of ClusterManagementService retrieved from the client side is * properly connected to the locator or not * * @return true if connected diff --git a/geode-management/src/main/java/org/apache/geode/management/api/RestfulEndpoint.java b/geode-management/src/main/java/org/apache/geode/management/api/RestfulEndpoint.java index 591c3991f7cf..fb01e2ce634f 100644 --- a/geode-management/src/main/java/org/apache/geode/management/api/RestfulEndpoint.java +++ b/geode-management/src/main/java/org/apache/geode/management/api/RestfulEndpoint.java @@ -15,6 +15,8 @@ package org.apache.geode.management.api; +import com.fasterxml.jackson.annotation.JsonIgnore; + public interface RestfulEndpoint { /** @@ -22,5 +24,6 @@ public interface RestfulEndpoint { * * @return e.g. /regions */ + @JsonIgnore String getEndpoint(); } diff --git a/geode-management/src/main/java/org/apache/geode/management/configuration/MemberConfig.java b/geode-management/src/main/java/org/apache/geode/management/configuration/MemberConfig.java new file mode 100644 index 000000000000..174d79a31a21 --- /dev/null +++ b/geode-management/src/main/java/org/apache/geode/management/configuration/MemberConfig.java @@ -0,0 +1,95 @@ +/* + * 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.configuration; + +import java.util.List; + +import org.apache.geode.annotations.Experimental; +import org.apache.geode.cache.configuration.CacheElement; +import org.apache.geode.management.api.RestfulEndpoint; + +@Experimental +public class MemberConfig implements CacheElement, RestfulEndpoint { + + private static final long serialVersionUID = -6262538068604902018L; + + public static final String MEMBER_CONFIG_ENDPOINT = "/members"; + + private boolean isLocator; + private boolean isCoordinator; + private String id; + private String host; + private String pid; + private List ports; + + public MemberConfig() { + + } + + public void setId(String id) { + this.id = id; + } + + public boolean isLocator() { + return isLocator; + } + + public void setLocator(boolean locator) { + isLocator = locator; + } + + public boolean isCoordinator() { + return isCoordinator; + } + + public void setCoordinator(boolean coordinator) { + isCoordinator = coordinator; + } + + public String getHost() { + return host; + } + + public void setHost(String host) { + this.host = host; + } + + public String getPid() { + return pid; + } + + public void setPid(String pid) { + this.pid = pid; + } + + public List getPorts() { + return ports; + } + + public void setPorts(List port) { + this.ports = port; + } + + @Override + public String getEndpoint() { + return MEMBER_CONFIG_ENDPOINT; + } + + @Override + public String getId() { + return id; + } +} diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java index f3d7cff52055..26592f0dae3f 100644 --- a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java +++ b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java @@ -15,6 +15,7 @@ package org.apache.geode.management.internal; + import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLContext; @@ -24,8 +25,6 @@ import org.apache.http.client.CredentialsProvider; import org.apache.http.impl.client.BasicCredentialsProvider; import org.apache.http.impl.client.HttpClientBuilder; -import org.springframework.http.HttpHeaders; -import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpRequestFactory; import org.springframework.http.client.HttpComponentsClientHttpRequestFactory; import org.springframework.web.client.ResponseErrorHandler; @@ -58,6 +57,8 @@ public class ClientClusterManagementService implements ClusterManagementService private static final ResponseErrorHandler DEFAULT_ERROR_HANDLER = new RestTemplateResponseErrorHandler(); + private static final String VERSION = "/v2"; + private final RestTemplate restTemplate; private ClientClusterManagementService() { @@ -75,7 +76,7 @@ public ClientClusterManagementService(String host, int port, SSLContext sslConte DefaultUriTemplateHandler templateHandler = new DefaultUriTemplateHandler(); String schema = (sslContext == null) ? "http" : "https"; - templateHandler.setBaseUrl(schema + "://" + host + ":" + port + "/geode-management/v2"); + templateHandler.setBaseUrl(schema + "://" + host + ":" + port + "/geode-management"); restTemplate.setUriTemplateHandler(templateHandler); // HttpComponentsClientHttpRequestFactory allows use to preconfigure httpClient for @@ -107,10 +108,9 @@ public ClientClusterManagementService(ClientHttpRequestFactory requestFactory) { @Override public ClusterManagementResult create(CacheElement config, String group) { String endPoint = getEndpoint(config); - HttpHeaders headers = new HttpHeaders(); - headers.setContentType(MediaType.APPLICATION_JSON); // the response status code info is represented by the ClusterManagementResult.errorCode already - return restTemplate.postForObject(endPoint, config, ClusterManagementResult.class); + return restTemplate.postForEntity(VERSION + endPoint, config, ClusterManagementResult.class) + .getBody(); } @Override @@ -123,6 +123,23 @@ public ClusterManagementResult update(CacheElement config, String group) { throw new NotImplementedException("Not Implemented"); } + @Override + public ClusterManagementResult list(CacheElement config) { + String endPoint = getEndpoint(config); + String id = config.getId(); + + // return restTemplate + // .getForEntity(VERSION + endPoint + ((id == null) ? "" : "/{id}"), + // ClusterManagementResult.class, id) + // .getBody(); + + return restTemplate + .getForEntity(VERSION + endPoint + ((id == null) ? "" : "/?id=" + id), + ClusterManagementResult.class) + .getBody(); + + } + public RestTemplate getRestTemplate() { return restTemplate; } @@ -138,7 +155,8 @@ private String getEndpoint(CacheElement config) { } public boolean isConnected() { - return restTemplate.getForObject("/ping", String.class).equals("pong"); + return restTemplate.getForEntity(VERSION + "/ping", String.class) + .getBody().equals("pong"); } } diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/cli/domain/ClassName.java b/geode-management/src/main/java/org/apache/geode/management/internal/cli/domain/ClassName.java index 362a4ce6d885..8510c8149750 100644 --- a/geode-management/src/main/java/org/apache/geode/management/internal/cli/domain/ClassName.java +++ b/geode-management/src/main/java/org/apache/geode/management/internal/cli/domain/ClassName.java @@ -21,10 +21,11 @@ import java.util.Properties; import java.util.regex.Pattern; -import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.lang3.StringUtils; +import org.apache.geode.util.internal.GeodeJsonMapper; + /** * This is mostly used for Gfsh command options that need to specify a className for instantiation. * @@ -35,11 +36,7 @@ public class ClassName implements Serializable { private String className = ""; private Properties initProperties = new Properties(); - private static ObjectMapper mapper = new ObjectMapper(); - - static { - mapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true); - } + private static ObjectMapper mapper = GeodeJsonMapper.getMapper(); // used to remove a Declarable through gfsh command // e.g. alter region --name=regionA --cache-loader='' diff --git a/geode-management/src/test/java/org/apache/geode/cache/configuration/CacheElementJsonMappingTest.java b/geode-management/src/test/java/org/apache/geode/cache/configuration/CacheElementJsonMappingTest.java new file mode 100644 index 000000000000..297fd4708cb3 --- /dev/null +++ b/geode-management/src/test/java/org/apache/geode/cache/configuration/CacheElementJsonMappingTest.java @@ -0,0 +1,88 @@ +/* + * 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.cache.configuration; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.configuration.MemberConfig; +import org.apache.geode.util.internal.GeodeJsonMapper; + +public class CacheElementJsonMappingTest { + private static ObjectMapper mapper = GeodeJsonMapper.getMapper(); + + private static MemberConfig member; + private static RegionConfig region; + + @BeforeClass + public static void beforeClass() throws Exception { + member = new MemberConfig(); + member.setId("server"); + member.setPid("123"); + + region = new RegionConfig(); + region.setName("test"); + } + + @Test + public void serializeRegion() throws Exception { + String json = mapper.writeValueAsString(region); + System.out.println(json); + assertThat(json).contains("class").contains("\"name\":\"test\""); + + RegionConfig config = mapper.readValue(json, RegionConfig.class); + assertThat(config.getName()).isEqualTo(region.getName()); + } + + @Test + public void serializeMember() throws Exception { + String json = mapper.writeValueAsString(member); + System.out.println(json); + assertThat(json).contains("class").contains("\"id\":\"server\""); + + MemberConfig config = mapper.readValue(json, MemberConfig.class); + assertThat(config.getId()).isEqualTo(member.getId()); + } + + @Test + public void serializeResult() throws Exception { + ClusterManagementResult result = new ClusterManagementResult(); + List elements = new ArrayList<>(); + elements.add(region); + elements.add(member); + result.setResult(elements); + + String json = mapper.writeValueAsString(result); + System.out.println(json); + + ClusterManagementResult result1 = mapper.readValue(json, ClusterManagementResult.class); + assertThat(result1.getResult()).hasSize(2); + } + + @Test + public void deserializeWithoutTypeInfo() throws Exception { + String json = "{'name':'test'}"; + RegionConfig config = mapper.readValue(json, RegionConfig.class); + assertThat(config.getName()).isEqualTo("test"); + } +} diff --git a/geode-web-management/build.gradle b/geode-web-management/build.gradle index d89057046fea..959ccf8de9be 100644 --- a/geode-web-management/build.gradle +++ b/geode-web-management/build.gradle @@ -17,11 +17,8 @@ apply plugin: 'war' apply from: "${project.projectDir}/../gradle/publish.gradle" -apply plugin: 'nebula.facet' -facets { - commonTest { - parentSourceSet = 'main' - } +configurations { + commonTestCompile } sourceSets { @@ -36,10 +33,19 @@ sourceSets { srcDir "${projectDir}/src/main/webapp" } } + + commonTest { + java { + srcDir "${projectDir}/src/commonTest/java" + } + compileClasspath = configurations.commonTestCompile + } } dependencies { - compile(platform(project(':boms:geode-all-bom'))) + compile(platform(project(':boms:geode-all-bom'))) { + exclude module: "jackson-annotations" + } compileOnly(project(':geode-core')) compileOnly('javax.servlet:javax.servlet-api') @@ -48,12 +54,13 @@ dependencies { compile('commons-fileupload:commons-fileupload') { exclude module: 'commons-io' } - compile('com.fasterxml.jackson.core:jackson-annotations') - compile('com.fasterxml.jackson.core:jackson-core') - compile('com.fasterxml.jackson.core:jackson-databind') - compile('com.fasterxml.jackson.module:jackson-module-scala_2.10') + compileOnly('com.fasterxml.jackson.core:jackson-annotations') + compileOnly('com.fasterxml.jackson.core:jackson-core') + compileOnly('com.fasterxml.jackson.core:jackson-databind') + compileOnly('com.fasterxml.jackson.module:jackson-module-scala_2.10') compile('io.springfox:springfox-swagger2') { exclude module: 'slf4j-api' + exclude module: "jackson-annotations" } compile('io.springfox:springfox-swagger-ui') { exclude module: 'slf4j-api' diff --git a/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/ClientClusterManagementServiceDUnitTest.java b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/ClientClusterManagementServiceDUnitTest.java index df2a9372ce31..e0c7f28403a1 100644 --- a/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/ClientClusterManagementServiceDUnitTest.java +++ b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/ClientClusterManagementServiceDUnitTest.java @@ -16,6 +16,8 @@ package org.apache.geode.management.client; +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -28,14 +30,12 @@ import org.springframework.test.web.client.MockMvcClientHttpRequestFactory; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; -import org.springframework.web.client.ResponseErrorHandler; import org.springframework.web.context.WebApplicationContext; import org.apache.geode.cache.RegionShortcut; import org.apache.geode.cache.configuration.RegionConfig; import org.apache.geode.management.api.ClusterManagementResult; import org.apache.geode.management.api.ClusterManagementService; -import org.apache.geode.management.internal.RestTemplateResponseErrorHandler; import org.apache.geode.management.internal.rest.BaseLocatorContextLoader; import org.apache.geode.management.internal.rest.PlainLocatorContextLoader; import org.apache.geode.test.dunit.rules.ClusterStartupRule; @@ -46,7 +46,6 @@ loader = PlainLocatorContextLoader.class) @WebAppConfiguration public class ClientClusterManagementServiceDUnitTest { - private static final ResponseErrorHandler ERROR_HANDLER = new RestTemplateResponseErrorHandler(); @Autowired private WebApplicationContext webApplicationContext; @@ -78,12 +77,6 @@ public void createRegion() { ClusterManagementResult result = client.create(region, ""); - // This all fails in light of running this test repeatedly as a stress test. Until we introduce - // idempotency and/or the ability to call client.delete we can't do this. But it will get fixed - // assertThat(result.isSuccessful()).isTrue(); - - // Not implemented yet - // result = client.delete(region, ""); - // assertThat(result.isSuccessful()).isTrue(); + assertThat(client.isConnected()).isTrue(); } } diff --git a/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceDUnitTest.java b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceDUnitTest.java new file mode 100644 index 000000000000..a0cd8f76fef3 --- /dev/null +++ b/geode-web-management/src/distributedTest/java/org/apache/geode/management/client/MemberManagementServiceDUnitTest.java @@ -0,0 +1,133 @@ +/* + * 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 org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.iterableWithSize; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +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.security.test.context.support.WithMockUser; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.context.web.WebAppConfiguration; +import org.springframework.test.web.client.MockMvcClientHttpRequestFactory; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.context.WebApplicationContext; + +import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.api.ClusterManagementService; +import org.apache.geode.management.configuration.MemberConfig; +import org.apache.geode.management.internal.rest.BaseLocatorContextLoader; +import org.apache.geode.management.internal.rest.PlainLocatorContextLoader; +import org.apache.geode.management.internal.rest.StandardRequestPostProcessor; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; + +@RunWith(SpringRunner.class) +@ContextConfiguration(locations = {"classpath*:WEB-INF/geode-management-servlet.xml"}, + loader = PlainLocatorContextLoader.class) +@WebAppConfiguration +public class MemberManagementServiceDUnitTest { + + static RequestPostProcessor POST_PROCESSOR = new StandardRequestPostProcessor(); + + @Autowired + private WebApplicationContext webApplicationContext; + + @Rule + public ClusterStartupRule cluster = new ClusterStartupRule(1); + + private MemberVM server1; + private ClusterManagementService client; + private MockMvc mockMvc; + + @Before + public void before() { + cluster.setSkipLocalDistributedSystemCleanup(true); + mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext) + .build(); + MockMvcClientHttpRequestFactory requestFactory = new MockMvcClientHttpRequestFactory(mockMvc); + client = ClusterManagementServiceProvider.getService(requestFactory); + + server1 = cluster.startServerVM(0, + BaseLocatorContextLoader.getLocatorFromContext(webApplicationContext).getPort()); + } + + @Test + @WithMockUser + public void listMember() { + MemberConfig memberConfig = new MemberConfig(); + ClusterManagementResult result = client.list(memberConfig); + + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(2); + } + + @Test + @WithMockUser + public void getOneMember() throws Exception { + MemberConfig config = new MemberConfig(); + config.setId("server-0"); + ClusterManagementResult result = client.list(config); + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(1); + } + + @Test + @WithMockUser + public void noMatchWithJavaAPI() throws Exception { + MemberConfig config = new MemberConfig(); + // look for a member with a non-existent id + config.setId("server"); + ClusterManagementResult result = client.list(config); + assertThat(result.isSuccessful()).isTrue(); + assertThat(result.getStatusCode()) + .isEqualTo(ClusterManagementResult.StatusCode.OK); + assertThat(result.getResult().size()).isEqualTo(0); + } + + @Test + @WithMockUser + public void noMatchWithFilter() throws Exception { + mockMvc.perform(get("/v2/members?id=server")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.statusCode", is("OK"))) + .andExpect(jsonPath("$.result", iterableWithSize(0))); + } + + @Test + @WithMockUser + public void noMatchWithUriVariable() throws Exception { + mockMvc.perform(get("/v2/members/server")) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.statusCode", is("ENTITY_NOT_FOUND"))) + .andExpect(jsonPath("$.statusMessage", + is("Unable to find the member with id = server"))); + } +} diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/MemberManagementController.java b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/MemberManagementController.java new file mode 100644 index 000000000000..4e4c07535039 --- /dev/null +++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/MemberManagementController.java @@ -0,0 +1,67 @@ +/* + * 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.rest.controllers; + +import static org.apache.geode.management.configuration.MemberConfig.MEMBER_CONFIG_ENDPOINT; +import static org.apache.geode.management.internal.rest.controllers.AbstractManagementController.MANAGEMENT_API_VERSION; + +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RequestParam; + +import org.apache.geode.management.api.ClusterManagementResult; +import org.apache.geode.management.configuration.MemberConfig; +import org.apache.geode.management.internal.exceptions.EntityNotFoundException; + +@Controller("members") +@RequestMapping(MANAGEMENT_API_VERSION) +public class MemberManagementController extends AbstractManagementController { + @PreAuthorize("@securityService.authorize('CLUSTER', 'READ')") + @RequestMapping(method = RequestMethod.GET, value = MEMBER_CONFIG_ENDPOINT + "/{id}") + public ResponseEntity getMember( + @PathVariable(name = "id", required = false) String id) { + MemberConfig config = new MemberConfig(); + config.setId(id); + ClusterManagementResult result = clusterManagementService.list(config); + + if (result.getResult().size() == 0) { + throw new EntityNotFoundException("Unable to find the member with id = " + id); + } + + return new ResponseEntity<>(result, + result.isSuccessful() ? HttpStatus.OK : HttpStatus.INTERNAL_SERVER_ERROR); + } + + @PreAuthorize("@securityService.authorize('CLUSTER', 'READ')") + @RequestMapping(method = RequestMethod.GET, value = MEMBER_CONFIG_ENDPOINT) + public ResponseEntity listMembers( + @RequestParam(required = false) String id) { + MemberConfig filter = new MemberConfig(); + if (id != null) { + filter.setId(id); + } + ClusterManagementResult result = clusterManagementService.list(filter); + + return new ResponseEntity<>(result, + result.isSuccessful() ? HttpStatus.OK : HttpStatus.INTERNAL_SERVER_ERROR); + } + +} diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/PingManagementController.java b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/PingManagementController.java new file mode 100644 index 000000000000..28ef4124b88a --- /dev/null +++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/PingManagementController.java @@ -0,0 +1,35 @@ +/* + * 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.rest.controllers; + + +import static org.apache.geode.management.internal.rest.controllers.AbstractManagementController.MANAGEMENT_API_VERSION; + +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; + +@Controller("ping") +@RequestMapping(MANAGEMENT_API_VERSION) +public class PingManagementController extends AbstractManagementController { + + @RequestMapping(method = RequestMethod.GET, value = "/ping") + public ResponseEntity ping() { + return new ResponseEntity<>("pong", HttpStatus.OK); + } +} diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/RegionManagementController.java b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/RegionManagementController.java index e59c0a191609..45333b28e5b1 100644 --- a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/RegionManagementController.java +++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/RegionManagementController.java @@ -15,6 +15,7 @@ package org.apache.geode.management.internal.rest.controllers; +import static org.apache.geode.cache.configuration.RegionConfig.REGION_CONFIG_ENDPOINT; import static org.apache.geode.management.internal.rest.controllers.AbstractManagementController.MANAGEMENT_API_VERSION; import org.springframework.http.HttpStatus; @@ -33,7 +34,7 @@ public class RegionManagementController extends AbstractManagementController { @PreAuthorize("@securityService.authorize('DATA', 'MANAGE')") - @RequestMapping(method = RequestMethod.POST, value = "/regions") + @RequestMapping(method = RequestMethod.POST, value = REGION_CONFIG_ENDPOINT) public ResponseEntity createRegion( @RequestBody RegionConfig regionConfig) { ClusterManagementResult result = @@ -41,9 +42,4 @@ public ResponseEntity createRegion( return new ResponseEntity<>(result, result.isSuccessful() ? HttpStatus.CREATED : HttpStatus.INTERNAL_SERVER_ERROR); } - - @RequestMapping(method = RequestMethod.GET, value = "/ping") - public ResponseEntity ping() { - return new ResponseEntity<>("pong", HttpStatus.OK); - } } diff --git a/geode-web-management/src/main/webapp/WEB-INF/geode-management-servlet.xml b/geode-web-management/src/main/webapp/WEB-INF/geode-management-servlet.xml index 22cae8efd9b0..a2cc9487c3ce 100644 --- a/geode-web-management/src/main/webapp/WEB-INF/geode-management-servlet.xml +++ b/geode-web-management/src/main/webapp/WEB-INF/geode-management-servlet.xml @@ -65,6 +65,7 @@ + diff --git a/geode-web-management/src/test/resources/expected-pom.xml b/geode-web-management/src/test/resources/expected-pom.xml index 5eec71a1228d..d9e364c26c2d 100644 --- a/geode-web-management/src/test/resources/expected-pom.xml +++ b/geode-web-management/src/test/resources/expected-pom.xml @@ -62,26 +62,6 @@ - - com.fasterxml.jackson.core - jackson-annotations - compile - - - com.fasterxml.jackson.core - jackson-core - compile - - - com.fasterxml.jackson.core - jackson-databind - compile - - - com.fasterxml.jackson.module - jackson-module-scala_2.10 - compile - io.springfox springfox-swagger2 @@ -91,6 +71,10 @@ slf4j-api * + + jackson-annotations + * +