pivotal-jbarrett commented on code in PR #7596:
URL: https://github.com/apache/geode/pull/7596#discussion_r851668162
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java:
##########
@@ -604,17 +611,23 @@ public void handleWANEvent(EntryEventImpl event) {
if (!(this instanceof AbstractBucketRegionQueue)) {
if (getBucketAdvisor().isPrimary()) {
+ if (notPrimary && needToWaitForColocatedBucketsBecomePrimary()) {
Review Comment:
`nonPrimary` appears to not be synchronized before access here.
##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java:
##########
@@ -30,11 +29,18 @@
import static org.mockito.Mockito.when;
import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
Review Comment:
Thanks for upgrading to JUnit5.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java:
##########
@@ -235,6 +237,12 @@ AtomicLong5 getEventSeqNum() {
return eventSeqNum;
}
+ boolean notPrimary = true;
+ volatile boolean allChildBucketsBecomePrimary = false;
+ private Object allChildBucketsBecomePrimaryLock = new Object();
+ volatile boolean alreadyInWaitForAllChildBucketsToBecomePrimary = false;
Review Comment:
All access to `alreadyInWaitForAllChildBucketsToBecomePrimary` is done under
synchronization so `volatile` is redundant.
##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java:
##########
@@ -153,12 +207,13 @@ public void
volunteerForPrimaryIgnoresMissingPrimaryElector() {
mock(BucketAdvisor.VolunteeringDelegate.class);
advisorSpy.setVolunteeringDelegate(volunteeringDelegate);
advisorSpy.initializePrimaryElector(missingElectorId);
- assertEquals(missingElectorId, advisorSpy.getPrimaryElector());
+ Assertions.assertEquals(missingElectorId, advisorSpy.getPrimaryElector());
Review Comment:
replace with `AssertJ`
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java:
##########
@@ -235,6 +237,12 @@ AtomicLong5 getEventSeqNum() {
return eventSeqNum;
}
+ boolean notPrimary = true;
Review Comment:
This new member is mutated and accessed under different synchronization
objects.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java:
##########
@@ -235,6 +237,12 @@ AtomicLong5 getEventSeqNum() {
return eventSeqNum;
}
+ boolean notPrimary = true;
+ volatile boolean allChildBucketsBecomePrimary = false;
Review Comment:
All access to `allChildBucketsBecomePrimary` is done under synchronization
so `volatile` is redundant.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java:
##########
@@ -235,6 +237,12 @@ AtomicLong5 getEventSeqNum() {
return eventSeqNum;
}
+ boolean notPrimary = true;
+ volatile boolean allChildBucketsBecomePrimary = false;
+ private Object allChildBucketsBecomePrimaryLock = new Object();
Review Comment:
Why are some of these members private and others not?
##########
geode-core/src/main/java/org/apache/geode/internal/cache/ColocationHelper.java:
##########
@@ -377,6 +378,26 @@ public static Map<String, LocalDataSet>
getColocatedLocalDataSetsForBuckets(
*/
public static List<PartitionedRegion> getColocatedChildRegions(
PartitionedRegion partitionedRegion) {
+ List<PartitionedRegion> colocatedChildRegions =
+ getColocatedChildRegions(partitionedRegion, true, false);
+
+ // Fix for 44484 - Make the list of colocated child regions
Review Comment:
Please remove bug ticket numbers in comments.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketAdvisor.java:
##########
@@ -2762,4 +2763,44 @@ void markShadowBucketAsDestroyed(String
shadowBucketPath) {
public boolean isShadowBucketDestroyed(String shadowBucketPath) {
return destroyedShadowBuckets.getOrDefault(shadowBucketPath, false);
}
+
+ 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()) {
+ return false;
+ } else {
+ if (!checkIfBecomesPrimary()) {
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+ }
+ return checkIfBecomesPrimary();
+ }
+
+ @NotNull
+ List<PartitionedRegion> getColocateNonShadowChildRegions() {
+ return ColocationHelper.getColocatedNonShadowChildRegions(pRegion);
+ }
+
+ private boolean checkIfBecomesPrimary() {
+ synchronized (this) {
+ try {
+ if (!isPrimary()) {
+ wait(10);
Review Comment:
Why the arbitrary wait 10ms?
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketAdvisor.java:
##########
@@ -2762,4 +2763,44 @@ void markShadowBucketAsDestroyed(String
shadowBucketPath) {
public boolean isShadowBucketDestroyed(String shadowBucketPath) {
return destroyedShadowBuckets.getOrDefault(shadowBucketPath, false);
}
+
+ boolean checkIfAllColocatedChildBucketsBecomePrimary() {
+ List<PartitionedRegion> colocatedChildPRs =
getColocateNonShadowChildRegions();
+ if (colocatedChildPRs.size() > 0) {
Review Comment:
Early exit.
```java
if (colocatedChildPRs.isEmpty()) {
return true;
}
```
--
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]
