This is an automated email from the ASF dual-hosted git repository.
jinmeiliao pushed a commit to branch support/1.13
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/support/1.13 by this push:
new f2e54fc GEODE-8055: create index command should work on sub regions
(#5034)
f2e54fc is described below
commit f2e54fc0484baca48d590859f0e8721196ca65d1
Author: Jinmei Liao <jil...@pivotal.io>
AuthorDate: Mon May 4 11:41:01 2020 -0700
GEODE-8055: create index command should work on sub regions (#5034)
---
.../geode/cache/configuration/CacheConfig.java | 14 ++++-
.../geode/cache/configuration/CacheConfigTest.java | 27 +++++++++
.../cli/commands/CreateIndexCommandDUnitTest.java | 49 ++++++++++++++++-
.../internal/cli/commands/CreateIndexCommand.java | 64 ++++++++++------------
.../cli/commands/CreateIndexCommandTest.java | 56 +++++++++++++------
...rConfigurationIndexWithFromClauseDUnitTest.java | 1 -
6 files changed, 153 insertions(+), 58 deletions(-)
diff --git
a/geode-core/src/main/java/org/apache/geode/cache/configuration/CacheConfig.java
b/geode-core/src/main/java/org/apache/geode/cache/configuration/CacheConfig.java
index 197ed05..aebb836 100644
---
a/geode-core/src/main/java/org/apache/geode/cache/configuration/CacheConfig.java
+++
b/geode-core/src/main/java/org/apache/geode/cache/configuration/CacheConfig.java
@@ -35,6 +35,7 @@ import org.w3c.dom.Element;
import org.apache.geode.annotations.Experimental;
import org.apache.geode.internal.config.VersionAdapter;
+import org.apache.geode.lang.Identifiable;
/**
* <p>
@@ -1024,11 +1025,22 @@ public class CacheConfig {
this.version = value;
}
+ // this supports looking for sub regions
public RegionConfig findRegionConfiguration(String regionPath) {
if (regionPath.startsWith(SEPARATOR)) {
regionPath = regionPath.substring(1);
}
- return find(getRegions(), regionPath);
+ List<RegionConfig> regions = getRegions();
+ RegionConfig found = null;
+ for (String regionToken : regionPath.split(SEPARATOR)) {
+ found = Identifiable.find(regions, regionToken);
+ // couldn't find one of the sub regions, break out of the loop
+ if (found == null) {
+ return null;
+ }
+ regions = found.getRegions();
+ }
+ return found;
}
public <T extends CacheElement> List<T> findCustomCacheElements(Class<T>
classT) {
diff --git
a/geode-core/src/test/java/org/apache/geode/cache/configuration/CacheConfigTest.java
b/geode-core/src/test/java/org/apache/geode/cache/configuration/CacheConfigTest.java
index 75473c2..88ff70c 100644
---
a/geode-core/src/test/java/org/apache/geode/cache/configuration/CacheConfigTest.java
+++
b/geode-core/src/test/java/org/apache/geode/cache/configuration/CacheConfigTest.java
@@ -221,4 +221,31 @@ public class CacheConfigTest {
cacheConfig.getRegions().add(regionConfig);
}
+
+ @Test
+ public void findRegionConfiguration() throws Exception {
+ CacheConfig config = new CacheConfig();
+ assertThat(config.findRegionConfiguration("/test")).isNull();
+ assertThat(config.findRegionConfiguration("test")).isNull();
+ assertThat(config.findRegionConfiguration("test/test1")).isNull();
+
+ RegionConfig testRegion = new RegionConfig();
+ testRegion.setName("test");
+ config.getRegions().add(testRegion);
+
+ assertThat(config.findRegionConfiguration("/test")).isNotNull();
+ assertThat(config.findRegionConfiguration("test")).isNotNull();
+ assertThat(config.findRegionConfiguration("test/test1")).isNull();
+
+ RegionConfig test1Region = new RegionConfig();
+ test1Region.setName("test1");
+ testRegion.getRegions().add(test1Region);
+
+ assertThat(config.findRegionConfiguration("/test")).isNotNull()
+ .extracting(RegionConfig::getName).isEqualTo("test");
+ assertThat(config.findRegionConfiguration("test")).isNotNull()
+ .extracting(RegionConfig::getName).isEqualTo("test");
+ assertThat(config.findRegionConfiguration("test/test1")).isNotNull()
+ .extracting(RegionConfig::getName).isEqualTo("test1");
+ }
}
diff --git
a/geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandDUnitTest.java
b/geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandDUnitTest.java
index 3266f39..aa1951c 100644
---
a/geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandDUnitTest.java
+++
b/geode-gfsh/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandDUnitTest.java
@@ -27,6 +27,7 @@ import org.junit.experimental.categories.Category;
import org.apache.geode.cache.Cache;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.cache.configuration.CacheConfig;
+import org.apache.geode.cache.configuration.RegionConfig;
import
org.apache.geode.distributed.internal.InternalConfigurationPersistenceService;
import org.apache.geode.test.dunit.IgnoredException;
import org.apache.geode.test.dunit.rules.ClusterStartupRule;
@@ -69,11 +70,53 @@ public class CreateIndexCommandDUnitTest {
}
@Test
+ public void createIndexOnSubRegion() throws Exception {
+ gfsh.executeAndAssertThat("create region --name=regionB/child
--group=group2 --type=REPLICATE")
+ .statusIsSuccess();
+
+ // make sure index on sub region can be created successfully
+ CommandResultAssert commandResultAssert =
+ gfsh.executeAndAssertThat(
+ "create index --name=childIndex --region='/regionB/child c'
--expression=id")
+ .statusIsSuccess();
+ commandResultAssert.hasTableSection()
+ .hasColumn("Message")
+ .containsExactly("Index successfully created");
+ commandResultAssert.hasInfoSection("groupStatus")
+ .hasOutput().isEqualTo("Cluster configuration for group 'group2' is
updated.");
+
+ // make sure cluster config is updated correctly
+ locator.invoke(() -> {
+ InternalConfigurationPersistenceService configurationService =
+ ClusterStartupRule.getLocator().getConfigurationPersistenceService();
+ CacheConfig cacheConfig = configurationService.getCacheConfig("group2");
+ assertThat(cacheConfig.getRegions()).extracting(RegionConfig::getName)
+ .containsExactly("regionB");
+
+ RegionConfig regionB = cacheConfig.getRegions().get(0);
+ assertThat(regionB.getRegions()).extracting(RegionConfig::getName)
+ .containsExactly("child");
+
+ assertThat(regionB.getRegions().get(0).getRegions()).isEmpty();
+ });
+ }
+
+ @Test
+ // index can't be created on region name with ".".
+ // GEODE-7523
+ public void createIndexOnRegionNameWithDot() throws Exception {
+ gfsh.executeAndAssertThat("create region --name=A.B --type=REPLICATE")
+ .statusIsSuccess();
+ gfsh.executeAndAssertThat("create index --name=indexWithDot --region=A.B
--expression=id")
+ .statusIsError().containsOutput("Region A does not exist");
+ }
+
+ @Test
public void regionNotExistInClusterConfig() {
gfsh.executeAndAssertThat("create index --name=myIndex --expression=id
--region=/noExist")
.statusIsError()
.hasInfoSection()
- .hasOutput().contains("Region /noExist does not exist");
+ .hasOutput().contains("Region noExist does not exist");
}
@Test
@@ -100,7 +143,7 @@ public class CreateIndexCommandDUnitTest {
gfsh.executeAndAssertThat("create index --name=myIndex --expression=id
--region=regionA")
.statusIsError()
.hasInfoSection()
- .hasOutput().contains("Region /regionA does not exist");
+ .hasOutput().contains("Region regionA does not exist");
// you can only create index on regionA when specifying a --member option
gfsh.executeAndAssertThat(
@@ -160,6 +203,6 @@ public class CreateIndexCommandDUnitTest {
.statusIsError();
commandAssert.hasInfoSection().hasOutput()
- .contains("Region /regionB does not exist in some of the groups.");
+ .contains("Region regionB does not exist in some of the groups.");
}
}
diff --git
a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommand.java
b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommand.java
index 0ba2070..79dacb5 100644
---
a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommand.java
+++
b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommand.java
@@ -19,23 +19,24 @@ import static
org.apache.geode.management.internal.cli.remote.CommandExecutor.RU
import static
org.apache.geode.management.internal.cli.remote.CommandExecutor.SERVICE_NOT_RUNNING_CHANGE_NOT_PERSISTED;
import java.util.Arrays;
+import java.util.HashSet;
import java.util.List;
-import java.util.Objects;
import java.util.Set;
-import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
import org.springframework.shell.core.annotation.CliCommand;
import org.springframework.shell.core.annotation.CliOption;
import org.apache.geode.annotations.Immutable;
+import org.apache.geode.cache.configuration.CacheConfig;
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.InternalConfigurationPersistenceService;
-import org.apache.geode.management.api.ClusterManagementService;
import org.apache.geode.management.cli.CliMetaData;
import org.apache.geode.management.cli.ConverterHint;
import org.apache.geode.management.cli.GfshCommand;
-import org.apache.geode.management.configuration.Region;
+import org.apache.geode.management.configuration.AbstractConfiguration;
import org.apache.geode.management.internal.cli.functions.CreateIndexFunction;
import org.apache.geode.management.internal.cli.result.model.InfoResultModel;
import org.apache.geode.management.internal.cli.result.model.ResultModel;
@@ -77,7 +78,6 @@ public class CreateIndexCommand extends GfshCommand {
// first find out what groups this region belongs to when using cluster
configuration
InternalConfigurationPersistenceService ccService =
getConfigurationPersistenceService();
- ClusterManagementService cms = getClusterManagementService();
final Set<DistributedMember> targetMembers;
ResultModel resultModel = new ResultModel();
InfoResultModel info = resultModel.addInfo();
@@ -85,8 +85,8 @@ public class CreateIndexCommand extends GfshCommand {
// if cluster management service is enabled and user did not specify a
member id, then
// we will find the applicable members based on the what group this region
is on
if (ccService != null && memberNameOrID == null) {
- regionName = getValidRegionName(regionPath, cms);
- Set<String> calculatedGroups = getGroupsContainingRegion(cms,
regionName);
+ regionName = getValidRegionName(regionPath);
+ Set<String> calculatedGroups = getGroupsContainingRegion(ccService,
regionName);
if (calculatedGroups.isEmpty()) {
return ResultModel.createError("Region " + regionName + " does not
exist.");
}
@@ -95,9 +95,11 @@ public class CreateIndexCommand extends GfshCommand {
.createError("Region " + regionName + " does not exist in some of
the groups.");
}
if (groups == null) {
- // the calculatedGroups will have null value to indicate the "cluster"
level, in thise case
+ // the calculatedGroups will have "cluster" value to indicate the
"cluster" level, in thise
+ // case
// we want the groups to an empty array
- groups =
calculatedGroups.stream().filter(Objects::nonNull).toArray(String[]::new);
+ groups = calculatedGroups.stream().filter(s ->
!AbstractConfiguration.CLUSTER.equals(s))
+ .toArray(String[]::new);
}
targetMembers = findMembers(groups, null);
}
@@ -132,8 +134,7 @@ public class CreateIndexCommand extends GfshCommand {
// update the cluster configuration. Can't use SingleGfshCommand to do the
update since in some
// cases
// groups information is inferred by the region, and the --group option
might have the wrong
- // group
- // information.
+ // group information.
if (ccService == null) {
info.addLine(SERVICE_NOT_RUNNING_CHANGE_NOT_PERSISTED);
return resultModel;
@@ -163,37 +164,28 @@ public class CreateIndexCommand extends GfshCommand {
// find a valid regionName when regionPath passed in is in the form of
// "/region1.fieldName.fieldName x"
- // this also handles the possibility when regionName has "." in it, like
"/A.B". It's stripping
- // . part one by one and check if the remaining part is a valid region name
or not. If we
- // could not find a region with any part of the name, (like, couldn't find
A.B or A), then A is
- // returned.
- String getValidRegionName(String regionPath, ClusterManagementService cms) {
- // Check to see if the region path contains an alias e.g "/region1 r1"
- // Then the first string will be the regionPath
+ // since we can't create index on regions with . in it's name, we will
assume tht regionName
+ // returned here should not have "."
+ String getValidRegionName(String regionPath) {
String regionName = regionPath.trim().split(" ")[0];
- // check to see if the region path is in the form of
"--region=region.entrySet() z"
- while (regionName.contains(".")) {
- Set<String> groupsContainingRegion = getGroupsContainingRegion(cms,
regionName);
- if (!groupsContainingRegion.isEmpty()) {
- break;
- }
- // otherwise, strip one more . part off the regionName
- else {
- regionName = regionName.substring(0, regionName.lastIndexOf("."));
- }
+ regionName = StringUtils.removeStart(regionName, "/");
+ if (regionName.contains(".")) {
+ regionName = regionName.substring(0, regionName.indexOf('.'));
}
return regionName;
}
- // if region belongs to "cluster" level, it will return a set of one null
value
- Set<String> getGroupsContainingRegion(ClusterManagementService cms,
+ // if region belongs to "cluster" level, it will return a set containing
"cluster" string
+ Set<String> getGroupsContainingRegion(ConfigurationPersistenceService cps,
String regionName) {
- Region regionConfig = new Region();
- regionConfig.setName(regionName);
- List<Region> regions = cms.list(regionConfig).getConfigResult();
- return regions.stream().map(Region::getGroup)
- .collect(Collectors.toSet());
+ Set<String> foundGroups = new HashSet<>();
+ for (String group : cps.getGroups()) {
+ CacheConfig cacheConfig = cps.getCacheConfig(group, true);
+ if (cacheConfig.findRegionConfiguration(regionName) != null) {
+ foundGroups.add(group);
+ }
+ }
+ return foundGroups;
}
-
}
diff --git
a/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandTest.java
b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandTest.java
index 7a5ae2d..41fc986 100644
---
a/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandTest.java
+++
b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/CreateIndexCommandTest.java
@@ -25,7 +25,9 @@ import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import java.util.Arrays;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -35,6 +37,7 @@ import org.junit.Rule;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
+import org.apache.geode.cache.configuration.CacheConfig;
import org.apache.geode.cache.configuration.RegionConfig;
import org.apache.geode.cache.execute.ResultCollector;
import org.apache.geode.distributed.DistributedMember;
@@ -149,20 +152,15 @@ public class CreateIndexCommandTest {
@Test
public void getValidRegionName() {
- // the existing configuration has a region named /regionA.B
-
doReturn(Collections.singleton("A")).when(command).getGroupsContainingRegion(cms,
- "/regionA.B");
- when(cms.list(any(Region.class))).thenReturn(new
ClusterManagementListResult<>());
-
- assertThat(command.getValidRegionName("regionB",
cms)).isEqualTo("regionB");
- assertThat(command.getValidRegionName("/regionB",
cms)).isEqualTo("/regionB");
- assertThat(command.getValidRegionName("/regionB b",
cms)).isEqualTo("/regionB");
- assertThat(command.getValidRegionName("/regionB.entrySet()", cms))
- .isEqualTo("/regionB");
- assertThat(command.getValidRegionName("/regionA.B.entrySet() A", cms))
- .isEqualTo("/regionA.B");
- assertThat(command.getValidRegionName("/regionA.fieldName.entrySet() B",
cms))
- .isEqualTo("/regionA");
+ assertThat(command.getValidRegionName("regionB")).isEqualTo("regionB");
+ assertThat(command.getValidRegionName("/regionB")).isEqualTo("regionB");
+ assertThat(command.getValidRegionName("/regionB b")).isEqualTo("regionB");
+ assertThat(command.getValidRegionName("/regionB.entrySet()"))
+ .isEqualTo("regionB");
+ assertThat(command.getValidRegionName("/regionA.B.entrySet() A"))
+ .isEqualTo("regionA");
+ assertThat(command.getValidRegionName("/regionA.fieldName.entrySet() B"))
+ .isEqualTo("regionA");
}
@Test
@@ -198,7 +196,7 @@ public class CreateIndexCommandTest {
gfshParser.executeAndAssertThat(command,
"create index --name=index --expression=abc --region=/regionA
--groups=group1,group3")
.statusIsError()
- .containsOutput("Region /regionA does not exist in some of the
groups");
+ .containsOutput("Region regionA does not exist in some of the groups");
verify(ccService, never()).updateCacheConfig(any(), any());
}
@@ -252,7 +250,7 @@ public class CreateIndexCommandTest {
when(cms.list(any(Region.class))).thenReturn(listResult);
when(listResult.getConfigResult()).thenReturn(Collections.singletonList(region));
- doReturn(Sets.newHashSet((String)
null)).when(command).getGroupsContainingRegion(any(),
+
doReturn(Sets.newHashSet("cluster")).when(command).getGroupsContainingRegion(any(),
any());
doReturn(Collections.emptySet()).when(command).findMembers(any(), any());
@@ -260,8 +258,32 @@ public class CreateIndexCommandTest {
"create index --name=index --expression=abc --region=/regionA")
.containsOutput("No Members Found");
-
verify(command).findMembers(new String[] {}, null);
+ }
+ @Test
+ public void getGroupsContainingRegion() throws Exception {
+ when(ccService.getGroups()).thenReturn(new
HashSet<>(Arrays.asList("group1", "group2")));
+ CacheConfig cacheConfig1 = new CacheConfig();
+ RegionConfig regionA = new RegionConfig("regionA", "REPLICATE");
+ RegionConfig regionB = new RegionConfig("regionB", "REPLICATE");
+ RegionConfig child = new RegionConfig("child", "REPLICATE");
+ regionA.getRegions().add(child);
+ cacheConfig1.getRegions().add(regionA);
+ cacheConfig1.getRegions().add(regionB);
+ CacheConfig cacheConfig2 = new CacheConfig();
+ cacheConfig2.getRegions().add(regionB);
+
+ when(ccService.getCacheConfig("group1", true)).thenReturn(cacheConfig1);
+ when(ccService.getCacheConfig("group2", true)).thenReturn(cacheConfig2);
+
+ assertThat(command.getGroupsContainingRegion(ccService, "regionA"))
+ .containsExactly("group1");
+ assertThat(command.getGroupsContainingRegion(ccService, "regionB"))
+ .containsExactlyInAnyOrder("group1", "group2");
+ assertThat(command.getGroupsContainingRegion(ccService, "regionA/child"))
+ .containsExactly("group1");
+ assertThat(command.getGroupsContainingRegion(ccService, "notExist"))
+ .isEmpty();
}
}
diff --git
a/geode-wan/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
b/geode-wan/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
index ba7bdd4..f6a55aa 100644
---
a/geode-wan/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
+++
b/geode-wan/src/distributedTest/java/org/apache/geode/management/internal/configuration/ClusterConfigurationIndexWithFromClauseDUnitTest.java
@@ -69,7 +69,6 @@ public class ClusterConfigurationIndexWithFromClauseDUnitTest
{
gfshCommandRule.connectAndVerify(locator);
createRegionUsingGfsh(REGION_NAME, regionShortcut, null);
createIndexUsingGfsh("\"" + REGION_NAME + ".entrySet() z\"", "z.key",
INDEX_NAME);
- String serverName = vm1.getName();
CommandStringBuilder csb = new
CommandStringBuilder(CliStrings.LIST_MEMBER);
gfshCommandRule.executeAndAssertThat(csb.toString()).statusIsSuccess();
lsRule.stop(1);
