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
