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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
