[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6373: GEODE-9191: PR clear could miss clearing bucket which lost primary

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

dschneider-pivotal commented on a change in pull request #6373:
URL: https://github.com/apache/geode/pull/6373#discussion_r620712461



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -165,8 +165,8 @@ protected void waitForPrimary(PartitionedRegion.RetryTimeKeeper retryTimer) {
                     new RegionEventImpl(localPrimaryBucketRegion, Operation.REGION_CLEAR, null,
                         false, partitionedRegion.getMyId(), regionEvent.getEventId());
                 localPrimaryBucketRegion.cmnClearRegion(bucketRegionEvent, false, true);
+                clearedBuckets.add(localPrimaryBucketRegion.getId());

Review comment:
       How is clearedBuckets used? You are changing the code to only add a bucket to it if the size of the bucket is > 0. I can see how clearedBuckets is used down at line 184 to decide if we will increment stats. This does not seem right. If we find a bunch of empty bucket then we still did a clear and took some time so it seems wrong to not even change the stats.
   Also we return clearedBuckets from this method and I can't tell what the callers do with it.
   Also this check that the primary bucket is not empty does not seem safe to me. Currently if evict destroy is configured on a PR then the primary and secondary can be out of sync. So the primary could be empty but the secondary could still have entries.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
##########
@@ -567,19 +567,13 @@ public boolean virtualPut(EntryEventImpl event, boolean ifNew, boolean ifOld,
    */
   @Override
   public void cmnClearRegion(RegionEventImpl regionEvent, boolean cacheWrite, boolean useRVV) {
-    if (!getBucketAdvisor().isPrimary()) {
-      if (logger.isDebugEnabled()) {
-        logger.debug("Not primary bucket when doing clear, do nothing");
-      }
-      return;
-    }
-
     // get rvvLock
     Set<InternalDistributedMember> participants =
         getCacheDistributionAdvisor().adviseInvalidateRegion();
     boolean isLockedAlready = this.partitionedRegion.getPartitionedRegionClear()
         .isLockedForListenerAndClientNotification();
 
+    boolean lockedForPrimary = doLockForPrimary(false);

Review comment:
       doLockForPrimary calls checkForPrimary which will throw PrimaryBucketException if this bucket is not a primary. Is that what you want? Before this method was a noop if it was a secondary. Do any of the tests cause this to be called on a secondary?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
##########
@@ -846,12 +846,12 @@ boolean processChunk(List entries, InternalDistributedMember sender)
       }
       List<Entry> entriesToSynchronize = new ArrayList<>();
 
+      if (internalDuringApplyDelta != null && !internalDuringApplyDelta.isRunning

Review comment:
       It looks to me like changing this test hook, by moving it out of the loop, will break the existing TombstoneDUnitTest that uses DuringApplyDelta. Why did you need to change it?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java
##########
@@ -567,19 +567,13 @@ public boolean virtualPut(EntryEventImpl event, boolean ifNew, boolean ifOld,
    */
   @Override
   public void cmnClearRegion(RegionEventImpl regionEvent, boolean cacheWrite, boolean useRVV) {
-    if (!getBucketAdvisor().isPrimary()) {
-      if (logger.isDebugEnabled()) {
-        logger.debug("Not primary bucket when doing clear, do nothing");
-      }
-      return;
-    }
-
     // get rvvLock
     Set<InternalDistributedMember> participants =
         getCacheDistributionAdvisor().adviseInvalidateRegion();
     boolean isLockedAlready = this.partitionedRegion.getPartitionedRegionClear()
         .isLockedForListenerAndClientNotification();
 
+    boolean lockedForPrimary = doLockForPrimary(false);

Review comment:
       Perhaps what you really want this method to do on a secondary is to just call clearRegionLocally(regionEvent, cacheWrite, null); All the locking and distribution stuff has been done on the primary




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

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