[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7596: GEODE-10242: Generate tail key after all child regions become primary

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

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: notifications-unsubscribe@geode.apache.org

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