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