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:
[email protected]
