[GitHub] [geode] kirklund commented on a change in pull request #6909: GEODE-9642: Wait for colocation completed at partitioned region initialization

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

kirklund commented on a change in pull request #6909:
URL: https://github.com/apache/geode/pull/6909#discussion_r720525647



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java
##########
@@ -1712,8 +1712,7 @@ private PartitionedRegion findPersistentRegionRecursively(PartitionedRegion part
   }
 
   void scheduleCreateMissingBuckets() {
-    if (partitionedRegion.getColocatedWith() != null
-        && ColocationHelper.isColocationComplete(partitionedRegion)) {
+    if (partitionedRegion.getColocatedWith() != null) {

Review comment:
       PRHARedundancyProviderTest should be updated with a test that fails without this change.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/CreateMissingBucketsTask.java
##########
@@ -18,18 +18,30 @@
 import org.apache.geode.internal.cache.PRHARedundancyProvider;
 import org.apache.geode.internal.cache.PartitionedRegion;
 import org.apache.geode.internal.cache.PartitionedRegion.RecoveryLock;
+import org.apache.geode.internal.cache.PartitionedRegionHelper;
 
 /**
  * A task for creating buckets in a child colocated region that are present in the leader region.
  *
  */
 public class CreateMissingBucketsTask extends RecoveryRunnable {
+  private static final int MAX_NUMBER_INTERVALS = 31;
+
+  private static final int SMALL_INTERVALS = 5;
+  private static final int MEDIUM_INTERVALS = 10;
+  private static final int LARGE_INTERVALS = 15;
+
   public CreateMissingBucketsTask(PRHARedundancyProvider prhaRedundancyProvider) {
     super(prhaRedundancyProvider);
   }
 
   @Override
   public void run2() {
+    if (!waitForColocationCompleted(redundancyProvider.getPartitionedRegion())) {
+      // if after all the time, colocation is still not completed, do nothing
+      return;
+    }
+

Review comment:
       Let's create a `CreateMissingBucketsTaskTest` (unit test) that tests this change. `ColocationHelper` makes the testing more difficult, but you can change `CreateMissingBucketsTask` to wrap the two static calls with `java.util.function.Function`s:
   ```
   public class CreateMissingBucketsTask extends RecoveryRunnable {
   
     private final Function<PartitionedRegion, PartitionedRegion> getLeaderRegionFunction;
     private final Function<PartitionedRegion, PartitionedRegion> getColocatedRegionFunction;
   
     public CreateMissingBucketsTask(PRHARedundancyProvider prhaRedundancyProvider) {
       this(prhaRedundancyProvider,
           ColocationHelper::getLeaderRegion,
           ColocationHelper::getColocatedRegion);
     }
   
     CreateMissingBucketsTask(PRHARedundancyProvider prhaRedundancyProvider,
                              Function<PartitionedRegion, PartitionedRegion> getLeaderRegionFunction,
                              Function<PartitionedRegion, PartitionedRegion> getColocatedRegionFunction) {
       super(prhaRedundancyProvider);
       this.getLeaderRegionFunction = getLeaderRegionFunction;
       this.getColocatedRegionFunction = getColocatedRegionFunction;
     }
   
     @Override
     public void run2() {
   //    PartitionedRegion leaderRegion =
   //        ColocationHelper.getLeaderRegion(redundancyProvider.getPartitionedRegion());
       PartitionedRegion leaderRegion =
           getLeaderRegionFunction.apply(redundancyProvider.getPartitionedRegion());
       RecoveryLock lock = leaderRegion.getRecoveryLock();
       lock.lock();
       try {
         createMissingBuckets(redundancyProvider.getPartitionedRegion());
       } finally {
         lock.unlock();
       }
     }
   
     protected void createMissingBuckets(PartitionedRegion region) {
   //    PartitionedRegion parentRegion = ColocationHelper.getColocatedRegion(region);
       PartitionedRegion parentRegion = getColocatedRegionFunction.apply(region);
       if (parentRegion == null) {
         return;
       }
       // Make sure the parent region has created missing buckets
       // before we create missing buckets for this child region.
       createMissingBuckets(parentRegion);
   
       for (int i = 0; i < region.getTotalNumberOfBuckets(); i++) {
   
         if (parentRegion.getRegionAdvisor().getBucketAdvisor(i).getBucketRedundancy() != region
             .getRegionAdvisor().getBucketAdvisor(i).getBucketRedundancy()) {
           region.getRedundancyProvider().createBucketAtomically(i, 0, true, null);
         }
       }
     }
   }
   ```

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/CreateMissingBucketsTask.java
##########
@@ -58,4 +70,38 @@ protected void createMissingBuckets(PartitionedRegion region) {
       }
     }
   }
+
+  /**
+   * Wait for Colocation to complete. Wait all nodes to Register this PartitionedRegion.
+   */
+  private boolean waitForColocationCompleted(PartitionedRegion partitionedRegion) {
+    int retryCount = 0;
+    int sleepInterval = PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION;
+
+    while (!ColocationHelper.isColocationComplete(partitionedRegion)

Review comment:
       `ColocationHelper.isColocationComplete` will need to wrapped in a `java.util.function.Function` as well to control this for unit testing.




-- 
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