gesterzhou commented on a change in pull request #6430:
URL: https://github.com/apache/geode/pull/6430#discussion_r641744823



##########
File path: 
geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -388,89 +375,105 @@ public void close(BucketRegion bucketRegion) {
     if (logger.isDebugEnabled()) {
       logger.debug("Clearing entries for {} rvv={}", _getOwner(), rvv);
     }
-    LocalRegion lr = _getOwner();
+    final LocalRegion lr = _getOwner();
     RegionVersionVector localRvv = lr.getVersionVector();
-    incClearCount(lr);
-    // lock for size calcs if the region might have tombstones
-    Object lockObj = lr.getConcurrencyChecksEnabled() ? lr.getSizeGuard() : 
new Object();
-    synchronized (lockObj) {
-      if (rvv == null) {
-        int delta = 0;
-        try {
-          delta = sizeInVM(); // TODO soplog need to determine if stats should
-                              // reflect only size in memory or the complete 
thing
-        } catch (GemFireIOException e) {
-          // ignore rather than throwing an exception during cache close
-        }
-        int tombstones = lr.getTombstoneCount();
-        _mapClear();
-        _getOwner().updateSizeOnClearRegion(delta - tombstones);
-        _getOwner().incTombstoneCount(-tombstones);
-        if (delta != 0) {
-          incEntryCount(-delta);
-        }
-      } else {
-        int delta = 0;
-        int tombstones = 0;
-        VersionSource myId = _getOwner().getVersionMember();
-        if (localRvv != rvv) {
-          localRvv.recordGCVersions(rvv);
-        }
-        final boolean isTraceEnabled = logger.isTraceEnabled();
-        for (RegionEntry re : regionEntries()) {
-          synchronized (re) {
-            Token value = re.getValueAsToken();
-            // if it's already being removed or the entry is being created we 
leave it alone
-            if (value == Token.REMOVED_PHASE1 || value == 
Token.REMOVED_PHASE2) {
-              continue;
-            }
 
-            VersionSource id = re.getVersionStamp().getMemberID();
-            if (id == null) {
-              id = myId;
-            }
-            if (rvv.contains(id, re.getVersionStamp().getRegionVersion())) {
-              if (isTraceEnabled) {
-                logger.trace("region clear op is removing {} {}", re.getKey(),
-                    re.getVersionStamp());
+    boolean isBucketRegion = false;
+    PartitionedRegionStats prStats = null;
+    long startTime = 0L;
+
+    if (lr.isUsedForPartitionedRegionBucket()) {
+      prStats = lr.getPartitionedRegion().getPrStats();
+      startTime = prStats.startBucketClear();
+      isBucketRegion = true;
+    }
+
+    try {
+      // lock for size calcs if the region might have tombstones
+      Object lockObj = lr.getConcurrencyChecksEnabled() ? lr.getSizeGuard() : 
new Object();
+      synchronized (lockObj) {
+        if (rvv == null) {
+          int delta = 0;
+          try {
+            delta = sizeInVM(); // TODO soplog need to determine if stats 
should
+            // reflect only size in memory or the complete thing
+          } catch (GemFireIOException e) {
+            // ignore rather than throwing an exception during cache close
+          }
+          int tombstones = lr.getTombstoneCount();
+          _mapClear();
+          _getOwner().updateSizeOnClearRegion(delta - tombstones);
+          _getOwner().incTombstoneCount(-tombstones);
+          if (delta != 0) {
+            incEntryCount(-delta);
+          }
+        } else {
+          int delta = 0;
+          int tombstones = 0;
+          VersionSource myId = _getOwner().getVersionMember();
+          if (localRvv != rvv) {
+            localRvv.recordGCVersions(rvv);
+          }
+          final boolean isTraceEnabled = logger.isTraceEnabled();
+          for (RegionEntry re : regionEntries()) {
+            synchronized (re) {
+              Token value = re.getValueAsToken();
+              // if it's already being removed or the entry is being created 
we leave it alone
+              if (value == Token.REMOVED_PHASE1 || value == 
Token.REMOVED_PHASE2) {
+                continue;
               }
 
-              boolean tombstone = re.isTombstone();
-              // note: it.remove() did not reliably remove the entry so we use 
remove(K,V) here
-              if (getEntryMap().remove(re.getKey(), re)) {
-                if (OffHeapClearRequired.doesClearNeedToCheckForOffHeap()) {
-                  GatewaySenderEventImpl.release(re.getValue()); // OFFHEAP 
_getValue ok
-                }
-                // If this is an overflow only region, we need to free the 
entry on
-                // disk at this point.
-                try {
-                  re.removePhase1(lr, true);
-                } catch (RegionClearedException e) {
-                  // do nothing, it's already cleared.
+              VersionSource id = re.getVersionStamp().getMemberID();
+              if (id == null) {
+                id = myId;
+              }
+              if (rvv.contains(id, re.getVersionStamp().getRegionVersion())) {
+                if (isTraceEnabled) {
+                  logger.trace("region clear op is removing {} {}", 
re.getKey(),
+                      re.getVersionStamp());
                 }
-                re.removePhase2();
-                lruEntryDestroy(re);
-                if (tombstone) {
-                  _getOwner().incTombstoneCount(-1);
-                  tombstones += 1;
-                } else {
-                  delta += 1;
+
+                boolean tombstone = re.isTombstone();
+                // note: it.remove() did not reliably remove the entry so we 
use remove(K,V) here
+                if (getEntryMap().remove(re.getKey(), re)) {
+                  if (OffHeapClearRequired.doesClearNeedToCheckForOffHeap()) {
+                    GatewaySenderEventImpl.release(re.getValue()); // OFFHEAP 
_getValue ok
+                  }
+                  // If this is an overflow only region, we need to free the 
entry on
+                  // disk at this point.
+                  try {
+                    re.removePhase1(lr, true);
+                  } catch (RegionClearedException e) {
+                    // do nothing, it's already cleared.
+                  }
+                  re.removePhase2();
+                  lruEntryDestroy(re);
+                  if (tombstone) {
+                    _getOwner().incTombstoneCount(-1);
+                    tombstones += 1;
+                  } else {
+                    delta += 1;
+                  }
                 }
+              } else { // rvv does not contain this entry so it is retained
+                result.add(id);
               }
-            } else { // rvv does not contain this entry so it is retained
-              result.add(id);
             }
           }
+          _getOwner().updateSizeOnClearRegion(delta);
+          incEntryCount(-delta);
+          incEntryCount(-tombstones);
+          if (logger.isDebugEnabled()) {
+            logger.debug("Size after clearing = {}", getEntryMap().size());
+          }
+          if (isTraceEnabled && getEntryMap().size() < 20) {
+            _getOwner().dumpBackingMap();
+          }
         }
-        _getOwner().updateSizeOnClearRegion(delta);
-        incEntryCount(-delta);
-        incEntryCount(-tombstones);
-        if (logger.isDebugEnabled()) {
-          logger.debug("Size after clearing = {}", getEntryMap().size());
-        }
-        if (isTraceEnabled && getEntryMap().size() < 20) {
-          _getOwner().dumpBackingMap();
-        }
+      }
+    } finally {
+      if (isBucketRegion) {
+        prStats.endBucketClear(startTime);
       }

Review comment:
       same. We might need "else" here




-- 
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:
us...@infra.apache.org


Reply via email to