DonalEvans commented on code in PR #7596:
URL: https://github.com/apache/geode/pull/7596#discussion_r857800483


##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketRegionTest.java:
##########
@@ -800,11 +808,11 @@ public void 
handleWANEventSetNotPrimaryIfWasPrimaryAndNoLongerAPrimary() {
 
     verify(bucketRegion).setNotPrimaryIfNecessary();
     assertThat(bucketRegion.notPrimary).isTrue();
-    assertThat(bucketRegion.allChildBucketsBecomePrimary).isFalse();
+    assertThat(bucketRegion.allColocatedBucketsBecomePrimary).isFalse();
   }
 
   @Test
-  public void handleWANEventDoesNotSetNotPrimaryIfWasNotPrimary() {
+  public void handleWANEventDoesNotSetNotPrimaryIfWas0NotPrimary() {

Review Comment:
   Typo here, I think. Should not have a "0" in the test name.



##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketAdvisor.java:
##########
@@ -2766,23 +2766,23 @@ public boolean isShadowBucketDestroyed(String 
shadowBucketPath) {
 
   boolean checkIfAllColocatedChildBucketsBecomePrimary() {
     List<PartitionedRegion> colocatedChildPRs = 
getColocateNonShadowChildRegions();
-    if (colocatedChildPRs.size() > 0) {
-      for (PartitionedRegion partitionedRegion : colocatedChildPRs) {
-        Bucket bucket = 
partitionedRegion.getRegionAdvisor().getBucket(getBucket().getId());
-        if (bucket != null) {
-          BucketAdvisor bucketAdvisor = bucket.getBucketAdvisor();
-          if (!bucketAdvisor.checkIfAllColocatedChildBucketsBecomePrimary()) {
+    if (colocatedChildPRs.isEmpty()) {
+      return checkIfBecomesPrimary();

Review Comment:
   I think that this can just be `return true;` since if there are no colocated 
child PRs, then it doesn't make sense to ever return false here.



##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketRegionTest.java:
##########
@@ -715,4 +722,178 @@ public void 
txHandleWANEventDoesNotCallHandleWANEventIfParallelWanNotEnabled() {
 
     verify(bucketRegion, never()).handleWANEvent(event);
   }
+
+  @Test
+  public void 
needToWaitForColocatedBucketsBecomePrimaryReturnsTrueIfHasChildRegionAndWasNotPrimary()
 {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    doReturn(true).when(bucketRegion).hasChildRegion();
+
+    
assertThat(bucketRegion.needToWaitForColocatedBucketsBecomePrimary()).isTrue();
+  }
+
+  @Test
+  public void 
needToWaitForColocatedBucketsBecomePrimaryReturnsFalseIfHasChildRegionAndWasPrimary()
 {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    doReturn(true).when(bucketRegion).hasChildRegion();
+    bucketRegion.notPrimary = false;
+
+    
assertThat(bucketRegion.needToWaitForColocatedBucketsBecomePrimary()).isFalse();
+  }
+
+  @Test
+  public void 
needToWaitForColocatedBucketsBecomePrimaryReturnsFalseIfNoChildRegion() {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    doReturn(false).when(bucketRegion).hasChildRegion();
+
+    
assertThat(bucketRegion.needToWaitForColocatedBucketsBecomePrimary()).isFalse();
+  }
+
+  @Test
+  public void 
handleWANEventDoesNotWaitForAllChildColocatedBucketsBecomePrimaryIfNoNeedToWait()
 {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    when(bucketAdvisor.isPrimary()).thenReturn(true);
+    
doReturn(false).when(bucketRegion).needToWaitForColocatedBucketsBecomePrimary();
+
+    when(partitionedRegion.getTotalNumberOfBuckets()).thenReturn(4);
+    doReturn(0).when(bucketRegion).getId();
+
+    bucketRegion.handleWANEvent(event);
+
+    verify(bucketRegion, 
never()).waitForAllChildColocatedBucketsBecomePrimary();
+    verify(event).setTailKey(4L);
+  }
+
+  @Test
+  public void 
handleWANEventWaitsForAllChildColocatedBucketsBecomePrimaryIfWasNotPrimary() {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    when(bucketAdvisor.isPrimary()).thenReturn(true);
+    doReturn(true).when(bucketRegion).hasChildRegion();
+    
doNothing().when(bucketRegion).waitForAllChildColocatedBucketsBecomePrimary();
+    when(partitionedRegion.getTotalNumberOfBuckets()).thenReturn(4);
+    doReturn(0).when(bucketRegion).getId();
+
+    bucketRegion.handleWANEvent(event);
+
+    verify(bucketRegion).waitForAllChildColocatedBucketsBecomePrimary();
+    verify(event).setTailKey(4L);
+  }
+
+  @Test
+  public void handleWANEventSetNotPrimaryIfWasPrimaryAndNoLongerAPrimary() {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    when(bucketAdvisor.isPrimary()).thenReturn(false);
+    bucketRegion.notPrimary = false;
+
+    bucketRegion.handleWANEvent(event);
+
+    verify(bucketRegion).setNotPrimaryIfNecessary();
+    assertThat(bucketRegion.notPrimary).isTrue();
+    assertThat(bucketRegion.allChildBucketsBecomePrimary).isFalse();
+  }
+
+  @Test
+  public void handleWANEventDoesNotSetNotPrimaryIfWasNotPrimary() {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    when(bucketAdvisor.isPrimary()).thenReturn(false);
+    bucketRegion.notPrimary = true;
+
+    bucketRegion.handleWANEvent(event);
+
+    verify(bucketRegion, never()).setNotPrimaryIfNecessary();
+  }
+
+  @Test
+  public void onlyOneThreadWillExecuteCheckIfAllChildBucketsBecomePrimary() 
throws Exception {
+    BucketRegion bucketRegion =
+        spy(new BucketRegion(regionName, regionAttributes, partitionedRegion,
+            cache, internalRegionArgs, disabledClock()));
+    
doNothing().when(bucketRegion).executeCheckIfAllChildBucketsBecomePrimary();
+
+    Future<?> future = 
executor.submit(bucketRegion::waitForAllChildColocatedBucketsBecomePrimary);
+    Future<?> future2 = 
executor.submit(bucketRegion::waitForAllChildColocatedBucketsBecomePrimary);
+
+    future.get();
+    future2.get();

Review Comment:
   None that I can think of, sorry. Since this test isn't actually testing what 
it says its testing, it should probably be renamed or reworked though. Maybe 
remove the async parts and just make it 
"multipleCallsToWaitForAllColocatedBucketsFromLeaderBecomePrimaryWillOnlyInvokeExecuteCheckIfAllChildBucketsFromLeaderBecomePrimaryOnce()"
 although that's a bit long.



-- 
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-unsubscr...@geode.apache.org

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

Reply via email to