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);