dschneider-pivotal commented on a change in pull request #7440:
URL: https://github.com/apache/geode/pull/7440#discussion_r827147415
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/DestroyRegionOperation.java
##########
@@ -167,32 +167,23 @@ public void run() {
Throwable thr = null;
try {
if (lclRgn == null) {
- // 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);
- } catch (PRLocallyDestroyedException ignore) {
- // region not found - it's been destroyed
+ PartitionedRegion partitionedRegion = PartitionedRegionHelper
+ .getPartitionedRegionUsingBucketRegionName(dm.getCache(),
regionPath);
+ String bucketName =
PartitionedRegionHelper.getBucketName(regionPath);
+ if (partitionedRegion != null && bucketName != null) {
Review comment:
since getPartitionedRegionUseBucektRegionName returns null if
getBucketName returns null, this if could be simplified to just check if
partitionedRegion != null. Move the bucketName initialization inside the "if".
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/RegionAdvisor.java
##########
@@ -180,19 +180,23 @@ public void processProfilesQueuedDuringInitialization() {
logger.trace(LogMarker.DISTRIBUTION_ADVISOR_VERBOSE,
"applying queued profile removal for all buckets for {}",
qbp.memberId);
}
- for (int i = 0; i < buckets.length; i++) {
- BucketAdvisor ba = buckets[i].getBucketAdvisor();
- int serial = qbp.serials[i];
- if (serial != ILLEGAL_SERIAL) {
- ba.removeIdWithSerial(qbp.memberId, serial, qbp.destroyed);
- }
- } // for
+ if (qbp.serials.length == 1) {
Review comment:
It is not clear to me why this change was made. It seems like the old
code required that the the "serials" array length was >= to the
"buckets.length". But now you allow "serials.length" to be 1 and in that case
do not check for ILLEGAL_SERIAL. What corresponding change required this one?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/RegionAdvisor.java
##########
@@ -426,6 +430,23 @@ public void removeIdAndBuckets(InternalDistributedMember
memberId, int prSerial,
}
}
+ public void removeIdAndBucket(int bucketId, InternalDistributedMember
memberId, int serial,
+ boolean regionDestroyed) {
+ synchronized (preInitQueueMonitor) {
+ if (preInitQueue != null) {
+ // Queue profile during pre-initialization
+ QueuedBucketProfile qbf =
+ new QueuedBucketProfile(bucketId, memberId, serial,
regionDestroyed);
+ preInitQueue.add(qbf);
+ return;
+ }
+ }
+
+ if (buckets != null) {
Review comment:
Is it really okay to just do nothing here if buckets is null? It does
not get nulled out at the end of its life. It is null after the constructor and
made non-null when initializeRegionAdvisor is called. But it is called BEFORE
we call processProfilesQueuedDuringInitialization which is the only place we
null out preInitQueue. We only get to this null check if preInitQueue is null.
My concern is if we do get to this line and buckets is null then something is
wrong with our internal logic and we are not going to call removeIdWithSerial
which seems wrong.
--
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
