DonalEvans commented on a change in pull request #5070: URL: https://github.com/apache/geode/pull/5070#discussion_r421757123
########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java ########## @@ -180,4 +189,75 @@ public void regionSyncNotInvokedInPerformSynchronizeForLostMemberTaskIfRegionNot verify(distributedRegion, never()).synchronizeForLostMember(member, lostMemberVersionID); } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenDispatcherIdCanNotBeFound() { + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(mock(GatewaySender.class))); + DistributedRegion distributedRegion = mock(DistributedRegion.class); Review comment: Would it be possible to have a spy `DistributedRegion` instead of a mock here? While using `doCallRealMethod()` works now because the `validateAsynchronousEventDispatcher()` method is entirely self-contained, if at some point in the future it's modified to access a field of `DistributedRegion` or call some other method in the class, the test will either fail, or pass despite not actually testing the true behaviour of the method. ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java ########## @@ -180,4 +189,75 @@ public void regionSyncNotInvokedInPerformSynchronizeForLostMemberTaskIfRegionNot verify(distributedRegion, never()).synchronizeForLostMember(member, lostMemberVersionID); } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenDispatcherIdCanNotBeFound() { + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(mock(GatewaySender.class))); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + distributedRegion.validateAsynchronousEventDispatcher("nonExistingDispatcher"); + } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenFoundDispatcherIsSerial() { + String senderId = "mySender"; + GatewaySender serialSender = mock(GatewaySender.class); + when(serialSender.isParallel()).thenReturn(false); + when(serialSender.getId()).thenReturn(senderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()).thenReturn(Collections.singleton(serialSender)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); Review comment: Would it be possible to use a spy rather than a mock here? ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java ########## @@ -180,4 +189,75 @@ public void regionSyncNotInvokedInPerformSynchronizeForLostMemberTaskIfRegionNot verify(distributedRegion, never()).synchronizeForLostMember(member, lostMemberVersionID); } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenDispatcherIdCanNotBeFound() { + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(mock(GatewaySender.class))); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + distributedRegion.validateAsynchronousEventDispatcher("nonExistingDispatcher"); + } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenFoundDispatcherIsSerial() { + String senderId = "mySender"; + GatewaySender serialSender = mock(GatewaySender.class); + when(serialSender.isParallel()).thenReturn(false); + when(serialSender.getId()).thenReturn(senderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()).thenReturn(Collections.singleton(serialSender)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + distributedRegion.validateAsynchronousEventDispatcher(senderId); + } + + @Test + public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatcherIdMatchesAnExistingParallelAsyncEventQueue() { + String senderId = "senderId"; + String regionPath = "thisRegion"; + String internalSenderId = getSenderIdFromAsyncEventQueueId(senderId); + GatewaySender parallelAsyncEventQueue = mock(GatewaySender.class); + when(parallelAsyncEventQueue.isParallel()).thenReturn(true); + when(parallelAsyncEventQueue.getId()).thenReturn(internalSenderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(parallelAsyncEventQueue)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + when(distributedRegion.getFullPath()).thenReturn(regionPath); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + assertThatThrownBy( + () -> distributedRegion.validateAsynchronousEventDispatcher(internalSenderId)) + .isInstanceOf(AsyncEventQueueConfigurationException.class) + .hasMessage("Parallel Async Event Queue " + senderId + + " can not be used with replicated region " + regionPath); + } + + @Test + public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatcherIdMatchesAnExistingParallelGatewaySender() { + String senderId = "senderId"; + String regionPath = "thisRegion"; + GatewaySender parallelGatewaySender = mock(GatewaySender.class); + when(parallelGatewaySender.isParallel()).thenReturn(true); + when(parallelGatewaySender.getId()).thenReturn(senderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(parallelGatewaySender)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); Review comment: Would it be possible to use a spy rather than a mock here? ########## File path: geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java ########## @@ -180,4 +189,75 @@ public void regionSyncNotInvokedInPerformSynchronizeForLostMemberTaskIfRegionNot verify(distributedRegion, never()).synchronizeForLostMember(member, lostMemberVersionID); } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenDispatcherIdCanNotBeFound() { + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(mock(GatewaySender.class))); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + distributedRegion.validateAsynchronousEventDispatcher("nonExistingDispatcher"); + } + + @Test + public void validateAsynchronousEventDispatcherShouldDoNothingWhenFoundDispatcherIsSerial() { + String senderId = "mySender"; + GatewaySender serialSender = mock(GatewaySender.class); + when(serialSender.isParallel()).thenReturn(false); + when(serialSender.getId()).thenReturn(senderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()).thenReturn(Collections.singleton(serialSender)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); + when(distributedRegion.getCache()).thenReturn(internalCache); + doCallRealMethod().when(distributedRegion).validateAsynchronousEventDispatcher(anyString()); + + distributedRegion.validateAsynchronousEventDispatcher(senderId); + } + + @Test + public void validateAsynchronousEventDispatcherShouldThrowExceptionWhenDispatcherIdMatchesAnExistingParallelAsyncEventQueue() { + String senderId = "senderId"; + String regionPath = "thisRegion"; + String internalSenderId = getSenderIdFromAsyncEventQueueId(senderId); + GatewaySender parallelAsyncEventQueue = mock(GatewaySender.class); + when(parallelAsyncEventQueue.isParallel()).thenReturn(true); + when(parallelAsyncEventQueue.getId()).thenReturn(internalSenderId); + InternalCache internalCache = mock(InternalCache.class); + when(internalCache.getAllGatewaySenders()) + .thenReturn(Collections.singleton(parallelAsyncEventQueue)); + DistributedRegion distributedRegion = mock(DistributedRegion.class); Review comment: Would it be possible to use a spy rather than a mock here? ########## File path: geode-wan/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/AlterRegionCommandDUnitTest.java ########## @@ -343,4 +346,76 @@ public void alterPartitionPersistentRegionWithParallelNonPersistentGatewaySender assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isBlank(); }); } + + @Test + public void alterReplicateRegionWithParallelGatewaySenderShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(GatewaySenderConfigurationException.class); + addIgnoredException("could not get remote locator information"); + String regionName = testName.getMethodName(); + String gatewaySenderId = testName.getMethodName() + "_parallelGatewaySender"; + + gfsh.executeAndAssertThat( + "create gateway-sender --parallel=true --enable-persistence=false --remote-distributed-system-id=2 --id=" Review comment: Could these tests use `CommandStringBuilder` to generate their gfsh commands? I find it makes things a little more readable. ########## File path: geode-wan/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/AlterRegionCommandDUnitTest.java ########## @@ -343,4 +346,76 @@ public void alterPartitionPersistentRegionWithParallelNonPersistentGatewaySender assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isBlank(); }); } + + @Test + public void alterReplicateRegionWithParallelGatewaySenderShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(GatewaySenderConfigurationException.class); + addIgnoredException("could not get remote locator information"); + String regionName = testName.getMethodName(); + String gatewaySenderId = testName.getMethodName() + "_parallelGatewaySender"; + + gfsh.executeAndAssertThat( + "create gateway-sender --parallel=true --enable-persistence=false --remote-distributed-system-id=2 --id=" + + gatewaySenderId) + .statusIsSuccess().doesNotContainOutput("Did not complete waiting"); + gfsh.executeAndAssertThat("create region --type=REPLICATE --name=" + regionName) + .statusIsSuccess(); + locator.waitUntilRegionIsReadyOnExactlyThisManyServers(SEPARATOR + regionName, 1); + + // Associate the gateway-sender + gfsh.executeAndAssertThat( + "alter region --name=" + regionName + " --gateway-sender-id=" + gatewaySenderId) + .statusIsError().containsOutput("server-1", "Parallel Gateway Sender " + gatewaySenderId + + " can not be used with replicated region " + SEPARATOR + regionName); + + // Check the cluster configuration service. + locator.invoke(() -> { + InternalLocator internalLocator = ClusterStartupRule.getLocator(); + assertThat(internalLocator).isNotNull(); + CacheConfig config = + internalLocator.getConfigurationPersistenceService().getCacheConfig("cluster"); + + RegionConfig regionConfig = find(config.getRegions(), regionName); + assertThat(regionConfig).isNotNull(); + assertThat(regionConfig.getRegionAttributes()).isNotNull(); + assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isNull(); + }); + } + + @Test + public void alterReplicateRegionWithParallelAsynchronousEventQueueShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(AsyncEventQueueConfigurationException.class); + String regionName = testName.getMethodName(); + String asyncEventQueueName = testName.getMethodName() + "_asyncEventQueue"; + + gfsh.executeAndAssertThat( + "create async-event-queue --parallel=true --persistent=false --listener=org.apache.geode.internal.cache.wan.MyAsyncEventListener --id=" Review comment: Another place to possibly use `CommandStringBuilder`. ########## File path: geode-wan/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/AlterRegionCommandDUnitTest.java ########## @@ -343,4 +346,76 @@ public void alterPartitionPersistentRegionWithParallelNonPersistentGatewaySender assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isBlank(); }); } + + @Test + public void alterReplicateRegionWithParallelGatewaySenderShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(GatewaySenderConfigurationException.class); + addIgnoredException("could not get remote locator information"); + String regionName = testName.getMethodName(); + String gatewaySenderId = testName.getMethodName() + "_parallelGatewaySender"; + + gfsh.executeAndAssertThat( + "create gateway-sender --parallel=true --enable-persistence=false --remote-distributed-system-id=2 --id=" + + gatewaySenderId) + .statusIsSuccess().doesNotContainOutput("Did not complete waiting"); + gfsh.executeAndAssertThat("create region --type=REPLICATE --name=" + regionName) + .statusIsSuccess(); + locator.waitUntilRegionIsReadyOnExactlyThisManyServers(SEPARATOR + regionName, 1); + + // Associate the gateway-sender + gfsh.executeAndAssertThat( + "alter region --name=" + regionName + " --gateway-sender-id=" + gatewaySenderId) Review comment: Another place to possibly use `CommandStringBuilder`. ########## File path: geode-wan/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/AlterRegionCommandDUnitTest.java ########## @@ -343,4 +346,76 @@ public void alterPartitionPersistentRegionWithParallelNonPersistentGatewaySender assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isBlank(); }); } + + @Test + public void alterReplicateRegionWithParallelGatewaySenderShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(GatewaySenderConfigurationException.class); + addIgnoredException("could not get remote locator information"); + String regionName = testName.getMethodName(); + String gatewaySenderId = testName.getMethodName() + "_parallelGatewaySender"; + + gfsh.executeAndAssertThat( + "create gateway-sender --parallel=true --enable-persistence=false --remote-distributed-system-id=2 --id=" + + gatewaySenderId) + .statusIsSuccess().doesNotContainOutput("Did not complete waiting"); + gfsh.executeAndAssertThat("create region --type=REPLICATE --name=" + regionName) + .statusIsSuccess(); + locator.waitUntilRegionIsReadyOnExactlyThisManyServers(SEPARATOR + regionName, 1); + + // Associate the gateway-sender + gfsh.executeAndAssertThat( + "alter region --name=" + regionName + " --gateway-sender-id=" + gatewaySenderId) + .statusIsError().containsOutput("server-1", "Parallel Gateway Sender " + gatewaySenderId + + " can not be used with replicated region " + SEPARATOR + regionName); + + // Check the cluster configuration service. + locator.invoke(() -> { + InternalLocator internalLocator = ClusterStartupRule.getLocator(); + assertThat(internalLocator).isNotNull(); + CacheConfig config = + internalLocator.getConfigurationPersistenceService().getCacheConfig("cluster"); + + RegionConfig regionConfig = find(config.getRegions(), regionName); + assertThat(regionConfig).isNotNull(); + assertThat(regionConfig.getRegionAttributes()).isNotNull(); + assertThat(regionConfig.getRegionAttributes().getGatewaySenderIds()).isNull(); + }); + } + + @Test + public void alterReplicateRegionWithParallelAsynchronousEventQueueShouldFailAndDoNotPersistChangesIntoTheClusterConfigurationService() { + addIgnoredException(AsyncEventQueueConfigurationException.class); + String regionName = testName.getMethodName(); + String asyncEventQueueName = testName.getMethodName() + "_asyncEventQueue"; + + gfsh.executeAndAssertThat( + "create async-event-queue --parallel=true --persistent=false --listener=org.apache.geode.internal.cache.wan.MyAsyncEventListener --id=" + + asyncEventQueueName) + .statusIsSuccess(); + locator.waitUntilAsyncEventQueuesAreReadyOnExactlyThisManyServers(asyncEventQueueName, 1); + + gfsh.executeAndAssertThat("create region --type=REPLICATE --name=" + regionName) + .statusIsSuccess(); + locator.waitUntilRegionIsReadyOnExactlyThisManyServers(SEPARATOR + regionName, 1); + + // Associate the async-event-queue + gfsh.executeAndAssertThat( + "alter region --name=" + regionName + " --async-event-queue-id=" + asyncEventQueueName) Review comment: Another place to possibly use `CommandStringBuilder`. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org