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


Reply via email to