This is an automated email from the ASF dual-hosted git repository.
onichols pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new a734c4e GEODE-5971: refactor DestroyRegionCommand and
CreateRegionCommand.interceptor (#3299)
a734c4e is described below
commit a734c4e91e055493458667c2ff9ca13302139423
Author: Owen Nichols <[email protected]>
AuthorDate: Thu Mar 14 09:26:05 2019 -0700
GEODE-5971: refactor DestroyRegionCommand and
CreateRegionCommand.interceptor (#3299)
* GEODE-5971: refactor DestroyRegionCommand and
CreateRegionCommand.interceptor to use ResultModel
---
.../internal/cli/commands/CreateRegionCommand.java | 37 ++++++------
.../cli/commands/DestroyRegionCommand.java | 30 ++++++----
.../cli/commands/CreateRegionCommandTest.java | 67 +++++++++-------------
.../cli/commands/DestroyRegionCommandTest.java | 14 ++---
4 files changed, 71 insertions(+), 77 deletions(-)
diff --git
a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
index de328cb..82dcabf 100644
---
a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
+++
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommand.java
@@ -53,7 +53,6 @@ import org.apache.geode.management.DistributedSystemMXBean;
import org.apache.geode.management.ManagementService;
import org.apache.geode.management.cli.CliMetaData;
import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
import org.apache.geode.management.cli.SingleGfshCommand;
import org.apache.geode.management.internal.cli.AbstractCliAroundInterceptor;
import org.apache.geode.management.internal.cli.GfshParseResult;
@@ -63,7 +62,6 @@ import
org.apache.geode.management.internal.cli.functions.CreateRegionFunctionAr
import
org.apache.geode.management.internal.cli.functions.FetchRegionAttributesFunction;
import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
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.management.internal.cli.util.RegionPath;
import org.apache.geode.management.internal.exceptions.EntityExistsException;
@@ -660,12 +658,12 @@ public class CreateRegionCommand extends
SingleGfshCommand {
public static class Interceptor extends AbstractCliAroundInterceptor {
@Override
- public Result preExecution(GfshParseResult parseResult) {
+ public ResultModel preExecution(GfshParseResult parseResult) {
Integer localMaxMemory =
(Integer)
parseResult.getParamValue(CliStrings.CREATE_REGION__LOCALMAXMEMORY);
if (localMaxMemory != null) {
if (localMaxMemory < 0) {
- return ResultBuilder.createUserErrorResult(
+ return ResultModel.createError(
"PartitionAttributes localMaxMemory must not be negative.");
}
}
@@ -673,7 +671,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
(Long)
parseResult.getParamValue(CliStrings.CREATE_REGION__TOTALMAXMEMORY);
if (totalMaxMemory != null) {
if (totalMaxMemory <= 0) {
- return ResultBuilder.createUserErrorResult(
+ return ResultModel.createError(
"Total size of partition region must be > 0.");
}
}
@@ -681,7 +679,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
(Integer)
parseResult.getParamValue(CliStrings.CREATE_REGION__REDUNDANTCOPIES);
if (redundantCopies != null) {
if (redundantCopies < 0 || redundantCopies > 3) {
- return ResultBuilder.createUserErrorResult(CliStrings.format(
+ return ResultModel.createError(CliStrings.format(
CliStrings.CREATE_REGION__MSG__REDUNDANT_COPIES_SHOULD_BE_ONE_OF_0123,
new Object[] {redundantCopies}));
}
@@ -691,7 +689,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
(Integer)
parseResult.getParamValue(CliStrings.CREATE_REGION__CONCURRENCYLEVEL);
if (concurrencyLevel != null) {
if (concurrencyLevel < 0) {
- return ResultBuilder.createUserErrorResult(CliStrings.format(
+ return ResultModel.createError(CliStrings.format(
CliStrings.CREATE_REGION__MSG__SPECIFY_POSITIVE_INT_FOR_CONCURRENCYLEVEL_0_IS_NOT_VALID,
new Object[] {concurrencyLevel}));
}
@@ -700,7 +698,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
String keyConstraint =
parseResult.getParamValueAsString(CliStrings.CREATE_REGION__KEYCONSTRAINT);
if (keyConstraint != null && !ClassName.isClassNameValid(keyConstraint))
{
- return ResultBuilder.createUserErrorResult(CliStrings.format(
+ return ResultModel.createError(CliStrings.format(
CliStrings.CREATE_REGION__MSG__SPECIFY_VALID_CLASSNAME_FOR_KEYCONSTRAINT_0_IS_INVALID,
new Object[] {keyConstraint}));
}
@@ -708,21 +706,21 @@ public class CreateRegionCommand extends
SingleGfshCommand {
String valueConstraint =
parseResult.getParamValueAsString(CliStrings.CREATE_REGION__VALUECONSTRAINT);
if (valueConstraint != null &&
!ClassName.isClassNameValid(valueConstraint)) {
- return ResultBuilder.createUserErrorResult(CliStrings.format(
+ return ResultModel.createError(CliStrings.format(
CliStrings.CREATE_REGION__MSG__SPECIFY_VALID_CLASSNAME_FOR_VALUECONSTRAINT_0_IS_INVALID,
new Object[] {valueConstraint}));
}
String compressor =
parseResult.getParamValueAsString(CliStrings.CREATE_REGION__COMPRESSOR);
if (compressor != null && !ClassName.isClassNameValid(compressor)) {
- return ResultBuilder.createUserErrorResult(CliStrings
+ return ResultModel.createError(CliStrings
.format(CliStrings.CREATE_REGION__MSG__INVALID_COMPRESSOR, new
Object[] {compressor}));
}
Boolean cloningEnabled =
(Boolean)
parseResult.getParamValue(CliStrings.CREATE_REGION__CLONINGENABLED);
if (compressor != null && cloningEnabled != null && !cloningEnabled) {
- return ResultBuilder.createUserErrorResult(CliStrings
+ return ResultModel.createError(CliStrings
.format(CliStrings.CREATE_REGION__MSG__CANNOT_DISABLE_CLONING_WITH_COMPRESSOR,
new Object[] {compressor}));
}
@@ -739,7 +737,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
+
CliStrings.format(CliStrings.CREATE_REGION__MSG__USE_ONE_OF_THESE_SHORTCUTS_0,
new Object[]
{String.valueOf(RegionCommandsUtils.PERSISTENT_OVERFLOW_SHORTCUTS)});
- return ResultBuilder.createUserErrorResult(message);
+ return ResultModel.createError(message);
}
}
@@ -773,7 +771,7 @@ public class CreateRegionCommand extends SingleGfshCommand {
&& (statisticsEnabled == null || !statisticsEnabled)) {
String message =
"Statistics must be enabled for expiration";
- return ResultBuilder.createUserErrorResult(message + ".");
+ return ResultModel.createError(message + ".");
}
@@ -786,27 +784,24 @@ public class CreateRegionCommand extends
SingleGfshCommand {
String evictionSizer =
parseResult.getParamValueAsString(CliStrings.CREATE_REGION__EVICTION_OBJECT_SIZER);
if (maxEntry != null && maxMemory != null) {
- return ResultBuilder
-
.createUserErrorResult(CliStrings.CREATE_REGION__MSG__BOTH_EVICTION_VALUES);
+ return
ResultModel.createError(CliStrings.CREATE_REGION__MSG__BOTH_EVICTION_VALUES);
}
if ((maxEntry != null || maxMemory != null) && evictionAction == null) {
- return ResultBuilder
-
.createUserErrorResult(CliStrings.CREATE_REGION__MSG__MISSING_EVICTION_ACTION);
+ return
ResultModel.createError(CliStrings.CREATE_REGION__MSG__MISSING_EVICTION_ACTION);
}
if (evictionSizer != null && maxEntry != null) {
- return ResultBuilder.createUserErrorResult(
+ return ResultModel.createError(
CliStrings.CREATE_REGION__MSG__INVALID_EVICTION_OBJECT_SIZER_AND_ENTRY_COUNT);
}
if (evictionAction != null
&& EvictionAction.parseAction(evictionAction) ==
EvictionAction.NONE) {
- return ResultBuilder
-
.createUserErrorResult(CliStrings.CREATE_REGION__MSG__INVALID_EVICTION_ACTION);
+ return
ResultModel.createError(CliStrings.CREATE_REGION__MSG__INVALID_EVICTION_ACTION);
}
- return ResultBuilder.createInfoResult("");
+ return ResultModel.createInfo("");
}
}
}
diff --git
a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommand.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommand.java
index 129a3de..0d233bd 100644
---
a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommand.java
+++
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommand.java
@@ -16,6 +16,7 @@ package org.apache.geode.management.internal.cli.commands;
import java.util.HashSet;
import java.util.List;
+import java.util.Objects;
import java.util.Set;
import org.springframework.shell.core.annotation.CliCommand;
@@ -28,23 +29,23 @@ import org.apache.geode.distributed.DistributedMember;
import
org.apache.geode.distributed.internal.InternalConfigurationPersistenceService;
import org.apache.geode.management.cli.CliMetaData;
import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.cli.GfshCommand;
import org.apache.geode.management.internal.cli.functions.CliFunctionResult;
import
org.apache.geode.management.internal.cli.functions.RegionDestroyFunction;
import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.apache.geode.management.internal.cli.result.CommandResult;
-import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.remote.CommandExecutor;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
import org.apache.geode.management.internal.configuration.domain.XmlEntity;
import org.apache.geode.management.internal.exceptions.EntityNotFoundException;
import org.apache.geode.management.internal.security.ResourceOperation;
import org.apache.geode.security.ResourcePermission;
-public class DestroyRegionCommand extends InternalGfshCommand {
+public class DestroyRegionCommand extends GfshCommand {
@CliCommand(value = {CliStrings.DESTROY_REGION}, help =
CliStrings.DESTROY_REGION__HELP)
@CliMetaData(relatedTopic = CliStrings.TOPIC_GEODE_REGION)
@ResourceOperation(resource = ResourcePermission.Resource.DATA,
operation = ResourcePermission.Operation.MANAGE)
- public Result destroyRegion(
+ public ResultModel destroyRegion(
@CliOption(key = CliStrings.DESTROY_REGION__REGION, optionContext =
ConverterHint.REGION_PATH,
mandatory = true, help = CliStrings.DESTROY_REGION__REGION__HELP)
String regionPath,
@CliOption(key = CliStrings.IFEXISTS, help = CliStrings.IFEXISTS_HELP,
@@ -65,7 +66,7 @@ public class DestroyRegionCommand extends InternalGfshCommand
{
try {
checkForJDBCMapping(regionPath);
} catch (IllegalStateException e) {
- return ResultBuilder.createGemFireErrorResult(e.getMessage());
+ return ResultModel.createError(e.getMessage());
}
// destroy is called on each member. If the region destroy is successful
on one member, we
@@ -75,18 +76,27 @@ public class DestroyRegionCommand extends
InternalGfshCommand {
executeAndGetFunctionResult(RegionDestroyFunction.INSTANCE,
regionPath, regionMembersList);
- CommandResult result = ResultBuilder.buildResult(resultsList);
+ ResultModel result = ResultModel.createMemberStatusResult(resultsList);
XmlEntity xmlEntity = findXmlEntity(resultsList);
// if at least one member returns with successful deletion, we will need
to update cc
+ InternalConfigurationPersistenceService configurationPersistenceService =
+ getConfigurationPersistenceService();
if (xmlEntity != null) {
- persistClusterConfiguration(result,
- () -> ((InternalConfigurationPersistenceService)
getConfigurationPersistenceService())
- .deleteXmlEntity(xmlEntity, null));
+ if (configurationPersistenceService == null) {
+
result.addInfo().addLine(CommandExecutor.SERVICE_NOT_RUNNING_CHANGE_NOT_PERSISTED);
+ } else {
+ configurationPersistenceService.deleteXmlEntity(xmlEntity, null);
+ }
}
return result;
}
+ private XmlEntity findXmlEntity(List<CliFunctionResult> functionResults) {
+ return functionResults.stream().filter(CliFunctionResult::isSuccessful)
+
.map(CliFunctionResult::getXmlEntity).filter(Objects::nonNull).findFirst().orElse(null);
+ }
+
void checkForJDBCMapping(String regionPath) {
String regionName = regionPath;
if (regionPath.startsWith("/")) {
diff --git
a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommandTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommandTest.java
index c451762..4822175 100644
---
a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommandTest.java
+++
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/CreateRegionCommandTest.java
@@ -37,11 +37,9 @@ import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.management.DistributedRegionMXBean;
import org.apache.geode.management.DistributedSystemMXBean;
import org.apache.geode.management.ManagementService;
-import org.apache.geode.management.cli.Result;
import org.apache.geode.management.internal.cli.GfshParseResult;
import org.apache.geode.management.internal.cli.domain.ClassName;
import
org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
-import org.apache.geode.management.internal.cli.result.CommandResult;
import org.apache.geode.test.junit.rules.GfshParserRule;
public class CreateRegionCommandTest {
@@ -81,54 +79,48 @@ public class CreateRegionCommandTest {
@Test
public void missingBothTypeAndUseAttributeFrom() throws Exception {
- CommandResult result =
- parser.executeCommandWithInstance(command, "create region
--name=region");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent())
+ parser.executeAndAssertThat(command, "create region --name=region")
+ .statusIsError()
+ .hasInfoSection().hasOutput()
.contains("One of \"type\" or \"template-region\" is required.");
}
@Test
public void haveBothTypeAndUseAttributeFrom() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE
--template-region=regionB");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent())
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--template-region=regionB").statusIsError()
+ .hasInfoSection().hasOutput()
.contains("Only one of type & template-region can be specified.");
}
@Test
public void invalidEvictionAction() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE
--eviction-action=invalidAction");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent())
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--eviction-action=invalidAction")
+ .statusIsError().hasInfoSection().hasOutput()
.contains("eviction-action must be 'local-destroy' or
'overflow-to-disk'");
}
@Test
public void invalidEvictionAttributes() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE
--eviction-max-memory=1000 --eviction-entry-count=200");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent())
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--eviction-max-memory=1000 --eviction-entry-count=200")
+ .statusIsError().hasInfoSection().hasOutput()
.contains("eviction-max-memory and eviction-entry-count cannot both be
specified.");
}
@Test
public void missingEvictionAction() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE
--eviction-max-memory=1000");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent()).contains("eviction-action must
be specified.");
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--eviction-max-memory=1000").statusIsError()
+ .hasInfoSection().hasOutput().contains("eviction-action must be
specified.");
}
@Test
public void invalidEvictionSizerAndCount() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE --eviction-entry-count=1
--eviction-object-sizer=abc --eviction-action=local-destroy");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent())
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE --eviction-entry-count=1
--eviction-object-sizer=abc --eviction-action=local-destroy")
+ .statusIsError().hasInfoSection().hasOutput()
.contains("eviction-object-sizer cannot be specified with
eviction-entry-count");
}
@@ -145,7 +137,7 @@ public class CreateRegionCommandTest {
doReturn(true).when(command).verifyDistributedRegionMbean(any(), any());
when(service.getDistributedRegionMXBean(any())).thenReturn(null);
- parser.executeCommandWithInstance(command, "create region --name=A
--type=REPLICATE");
+ parser.executeAndAssertThat(command, "create region --name=A
--type=REPLICATE").statusIsError();
ArgumentCaptor<CreateRegionFunctionArgs> argsCaptor =
ArgumentCaptor.forClass(CreateRegionFunctionArgs.class);
verify(command).executeFunction(any(), argsCaptor.capture(),
any(Set.class));
@@ -278,26 +270,23 @@ public class CreateRegionCommandTest {
@Test
public void invalidCompressor() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE --compressor=abc-def");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent()).contains("abc-def is an invalid
Compressor.");
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--compressor=abc-def").statusIsError()
+ .hasInfoSection().hasOutput().contains("abc-def is an invalid
Compressor.");
}
@Test
public void invalidKeyType() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE --key-type=abc-def");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent()).contains("Invalid command");
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--key-type=abc-def").statusIsError()
+ .hasInfoSection().hasOutput().contains("Invalid command");
}
@Test
public void invalidValueType() throws Exception {
- CommandResult result = parser.executeCommandWithInstance(command,
- "create region --name=region --type=REPLICATE --value-type=abc-def");
- assertThat(result.getStatus()).isEqualTo(Result.Status.ERROR);
- assertThat(result.getMessageFromContent()).contains("Invalid command");
+ parser.executeAndAssertThat(command,
+ "create region --name=region --type=REPLICATE
--value-type=abc-def").statusIsError()
+ .hasInfoSection().hasOutput().contains("Invalid command");
}
@Test
diff --git
a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommandTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommandTest.java
index 46d069d..f922d39 100644
---
a/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommandTest.java
+++
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DestroyRegionCommandTest.java
@@ -113,11 +113,11 @@ public class DestroyRegionCommandTest {
doReturn(Collections.singleton(DistributedMember.class)).when(command)
.findMembersForRegion(any());
when(result1.isSuccessful()).thenReturn(true);
- when(result1.getLegacyStatus()).thenReturn("result1 message");
+ when(result1.getStatusMessage()).thenReturn("result1 message");
when(result1.getXmlEntity()).thenReturn(xmlEntity);
when(result2.isSuccessful()).thenReturn(false);
- when(result2.getLegacyStatus()).thenReturn("result2 message");
+ when(result2.getStatusMessage()).thenReturn("result2 message");
parser.executeAndAssertThat(command, "destroy region
--name=test").statusIsSuccess()
.containsOutput("result1 message").containsOutput("result2 message");
@@ -132,11 +132,11 @@ public class DestroyRegionCommandTest {
doReturn(Collections.singleton(DistributedMember.class)).when(command)
.findMembersForRegion(any());
when(result1.isSuccessful()).thenReturn(true);
- when(result1.getLegacyStatus()).thenReturn("result1 message");
+ when(result1.getStatusMessage()).thenReturn("result1 message");
when(result1.getXmlEntity()).thenReturn(xmlEntity);
when(result2.isSuccessful()).thenReturn(false);
- when(result2.getLegacyStatus()).thenReturn("something happened");
+ when(result2.getStatusMessage()).thenReturn("something happened");
parser.executeAndAssertThat(command, "destroy region
--name=test").statusIsSuccess()
.containsOutput("result1 message").containsOutput("something
happened");
@@ -152,17 +152,17 @@ public class DestroyRegionCommandTest {
doReturn(Collections.singleton(DistributedMember.class)).when(command)
.findMembersForRegion(any());
when(result1.isSuccessful()).thenReturn(false);
- when(result1.getLegacyStatus()).thenReturn("result1 message");
+ when(result1.getStatusMessage()).thenReturn("result1 message");
when(result2.isSuccessful()).thenReturn(false);
- when(result2.getLegacyStatus()).thenReturn("something happened");
+ when(result2.getStatusMessage()).thenReturn("something happened");
parser.executeAndAssertThat(command, "destroy region
--name=test").statusIsError()
.containsOutput("result1 message").containsOutput("something
happened");
// verify that xmlEntry returned by the result1 is saved to Cluster config
- verify(command, never()).persistClusterConfiguration(any(), any());
+ verify(ccService, never()).deleteXmlEntity(any(), any());
}
private void setupJDBCMappingOnRegion(String regionName) {
