[GitHub] [geode] DonalEvans commented on a change in pull request #7144: GEODE-9853: get all members hosting bucket

[GitBox <gi...@apache.org>]

DonalEvans commented on a change in pull request #7144:
URL: https://github.com/apache/geode/pull/7144#discussion_r760658369



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##########
@@ -1981,6 +1982,36 @@ public void run() {
       return recipients;
     }
 
+
+    private Set<InternalDistributedMember> getAllRecipientsForEvent(InternalCache cache,
+        String partitionedRegionName, int bucketId) {
+      Set recipients = new ObjectOpenHashSet();

Review comment:
       Compiler warnings here can be fixed by using `Set<InternalDistributedMember> recipients = new ObjectOpenHashSet<>();`

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##########
@@ -1206,7 +1206,8 @@ public void sendQueueRemovalMesssageForDroppedEvent(PartitionedRegion prQ, int b
     temp.put(prQ.getFullPath(), bucketIdToDispatchedKeys);
     addRemovedEventToMap(bucketIdToDispatchedKeys, bucketId, key);
     Set<InternalDistributedMember> recipients =
-        removalThread.getAllRecipients(sender.getCache(), temp);
+        removalThread.getAllRecipientsForEvent(sender.getCache(), prQ.getFullPath(), bucketId);

Review comment:
       This method can be optimized by moving everything above this line inside the if statement. That way, if there are no recipients, then we don't do all the extra allocations and work for nothing.

##########
File path: geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1514,10 +1596,34 @@ private void waitForSendersRunning() {
     vm7.invoke(() -> waitForSenderRunningState("ln"));
   }
 
+  private void waitForSenderNonRunning() {

Review comment:
       This would be better named "waitForAllSendersNotRunning"

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java
##########
@@ -228,4 +228,53 @@ public void isShadowBucketDestroyedShouldReturnCorrectly() {
     // Unknown Shadow Bucket
     assertThat(bucketAdvisor.isShadowBucketDestroyed(SEPARATOR + "b5")).isFalse();
   }
+
+  @Test
+  public void testGetAllHostingMembersReturnsNoMember() throws Exception {
+    DistributionManager distributionManager = mock(DistributionManager.class);
+    when(distributionManager.getId()).thenReturn(new InternalDistributedMember("localhost", 321));
+
+    Bucket bucket = mock(Bucket.class);
+    when(bucket.isHosting()).thenReturn(true);
+    when(bucket.isPrimary()).thenReturn(false);
+    when(bucket.getDistributionManager()).thenReturn(distributionManager);
+
+    PartitionedRegion partitionedRegion = mock(PartitionedRegion.class);
+    when(partitionedRegion.getRedundantCopies()).thenReturn(0);
+    when(partitionedRegion.getPartitionAttributes()).thenReturn(new PartitionAttributesImpl());
+    RegionAdvisor regionAdvisor = mock(RegionAdvisor.class);
+    when(regionAdvisor.getPartitionedRegion()).thenReturn(partitionedRegion);
+
+    BucketAdvisor bucketAdvisor = BucketAdvisor.createBucketAdvisor(bucket, regionAdvisor);
+    assertThat(bucketAdvisor.getAllHostingMembers().isEmpty()).isTrue();
+  }
+
+  @Test
+  public void testGetAllHostingMembersReturnsOneMember() throws Exception {

Review comment:
       `Exception` is never thrown from this method, so this can be removed.

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java
##########
@@ -228,4 +228,53 @@ public void isShadowBucketDestroyedShouldReturnCorrectly() {
     // Unknown Shadow Bucket
     assertThat(bucketAdvisor.isShadowBucketDestroyed(SEPARATOR + "b5")).isFalse();
   }
+
+  @Test
+  public void testGetAllHostingMembersReturnsNoMember() throws Exception {

Review comment:
       `Exception` is never thrown from this method, so this can be removed. 
   
   Also, this test name could be more descriptive by including the conditions that lead to this behaviour. As it currently is, it implies that we expect that `getAllHostingMembers` always returns no members, which is confusing when considering the other test below, which implies the opposite. Maybe something like "getAllHostingMembersReturnsNoMembersWhenBucketAdvisorHasNoProfiles" for this test and "getAllHostingMembersReturnsMemberWhenBucketAdvisorHasOneProfileWithHostingBucket".
   
   It would also be good to add a test for the case where the bucket advisor has two profiles; one for a bucket which is  hosting and one for a bucket which is not hosting, to show that only the first is returned.

##########
File path: geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1185,6 +1188,85 @@ public void testParallelGWSenderUpdateFiltersWhilePuttingOnOneDispatcThread() th
 
   }
 
+  /**
+   * Put entries in region after gateway sander is stopped. Count number of PQRM messages sent.
+   */
+  @Test
+  public void testDroppedEventsSignalizationToSecundaryQueueWhileSenderStopped() throws Exception {
+    int[] locatorPorts = createLNAndNYLocatorsReturnPrimitives();
+    int lnPort = locatorPorts[0];
+    int nyPort = locatorPorts[1];

Review comment:
       Since the `createLNAndNYLocatorsReturnPrimitives()` method is only used here, it's probably simpler to just inline it:
   ```
       int lnPort = vm0.invoke(() -> createFirstLocatorWithDSId(1));
       int nyPort = vm1.invoke(() -> createFirstRemoteLocator(2, lnPort));
   ```

##########
File path: geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1185,6 +1188,85 @@ public void testParallelGWSenderUpdateFiltersWhilePuttingOnOneDispatcThread() th
 
   }
 
+  /**
+   * Put entries in region after gateway sander is stopped. Count number of PQRM messages sent.
+   */
+  @Test
+  public void testDroppedEventsSignalizationToSecundaryQueueWhileSenderStopped() throws Exception {

Review comment:
       `Exception` is never thrown from this method, so this can be removed.




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org