dschneider-pivotal commented on a change in pull request #7364: URL: https://github.com/apache/geode/pull/7364#discussion_r819178039
########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DestroyRegionOperation.java ########## @@ -170,11 +174,15 @@ public void run() { // following block is specific to buckets... // need to wait for queued bucket profiles to be processed // or this destroy may do nothing and result in a stale profile - boolean waitForBucketInitializationToComplete = true; CacheDistributionAdvisee advisee = null; try { Review comment: I think you should bring back the "waitForBucketInitializationToComplete = true;" and then add your new "if" that sets this flag to false. I also think you should add a comment why it needs to be set to false. After the "if" you will have a single call of getProxyBucketRegion that uses the boolean flag. Or you could have "waitForBucketInitializationToComplete = !op.isLocal() || op.isClose() || !op.isRegionDestroy()" ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DestroyRegionOperation.java ########## @@ -92,8 +92,12 @@ public DestroyRegionOperation(RegionEventImpl event, boolean notifyOfRegionDepar @Override protected Set getRecipients() { - CacheDistributionAdvisor advisor = getRegion().getCacheDistributionAdvisor(); - return advisor.adviseDestroyRegion(); + if (getRegion() instanceof BucketRegion) { Review comment: instead of having this "instanceof" check here, I think it would be better to add a method on DistributedRegion named getDestroyRegionRecipients and have it do the else part of this method and then override that method on BucketRegion and have it do the "if" part of this method. And then this method can just be "return getRegion().getDestroyRegionRecipients();". Also, if it would not be too much work, try to change the returned "Set" to be "Set<InternalDistributedMember>" ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DestroyRegionOperation.java ########## @@ -170,11 +174,15 @@ public void run() { // following block is specific to buckets... // need to wait for queued bucket profiles to be processed // or this destroy may do nothing and result in a stale profile - boolean waitForBucketInitializationToComplete = true; CacheDistributionAdvisee advisee = null; try { - advisee = PartitionedRegionHelper.getProxyBucketRegion(dm.getCache(), regionPath, - waitForBucketInitializationToComplete); + if (op.isRegionDestroy() && !op.isClose() && op.isLocal()) { Review comment: It seems like this could be simplified to "if (!op.isClose())". ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/DestroyRegionOperation.java ########## @@ -170,11 +174,15 @@ public void run() { // following block is specific to buckets... // need to wait for queued bucket profiles to be processed // or this destroy may do nothing and result in a stale profile - boolean waitForBucketInitializationToComplete = true; CacheDistributionAdvisee advisee = null; try { Review comment: If I'm write about how the "if" condition can be simplified then this could be "waitForBucketInitializationToComplete = op.isClose()" -- 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-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org