You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by mh...@apache.org on 2020/08/14 00:08:57 UTC
[geode] branch support/1.13 updated: Changes to help reduce the
occurence of GEODE-8422 (#5447)
This is an automated email from the ASF dual-hosted git repository.
mhanson pushed a commit to branch support/1.13
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/support/1.13 by this push:
new 0f8500a Changes to help reduce the occurence of GEODE-8422 (#5447)
0f8500a is described below
commit 0f8500a4cfa3c9ad575ddd41b37f0a78b4569cc1
Author: mhansonp <ha...@vmware.com>
AuthorDate: Wed Aug 12 16:34:47 2020 -0700
Changes to help reduce the occurence of GEODE-8422 (#5447)
* Changes to help reduce the occurence of GEODE-8422
* Added a unit test for the case where we don't want
to remove a tombstone with the region is not initialized.
* fix the dunit tests' behavior
Co-authored-by: agingade <ag...@vmware.com>
Co-authored-by: zhouxh < zhouxh@vmware.com>
(cherry picked from commit cf27a6a68f0ff89513e4c3a0c0a7a6235ab61e7f)
---
.../geode/internal/cache/GIIDeltaDUnitTest.java | 15 ++--
.../geode/internal/cache/TombstoneService.java | 28 ++++---
.../geode/internal/cache/TombstoneServiceTest.java | 89 ++++++++++++++++++++++
3 files changed, 113 insertions(+), 19 deletions(-)
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/GIIDeltaDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/GIIDeltaDUnitTest.java
index 26a979c..0ec9ceb 100644
--- a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/GIIDeltaDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/GIIDeltaDUnitTest.java
@@ -497,7 +497,7 @@ public class GIIDeltaDUnitTest extends JUnit4CacheTestCase {
// If fullGII, the key size in gii chunk is 3, i.e. key1,key3,key5. key2 is GCed.
// If delta GII, the key size should be 1 (key5(T) which is unfinished operation)
- verifyDeltaSizeFromStats(R, 3, 0);
+ verifyDeltaSizeFromStats(R, 1, 1);
}
/**
@@ -1534,9 +1534,9 @@ public class GIIDeltaDUnitTest extends JUnit4CacheTestCase {
assertEquals(0, count);
verifyDeltaSizeFromStats(R, 1, 1); // deltaGII, key1 in delta
- // tombstone key2, key5 should be GCed at R
- verifyTombstoneExist(R, "key2", false, false);
- verifyTombstoneExist(R, "key5", false, false);
+ // tombstone key2, key5 should still exist and expired at R
+ verifyTombstoneExist(R, "key2", true, true);
+ verifyTombstoneExist(R, "key5", true, true);
// tombstone key2, key5 should still exist and expired at P
verifyTombstoneExist(P, "key2", true, true);
@@ -1544,10 +1544,10 @@ public class GIIDeltaDUnitTest extends JUnit4CacheTestCase {
RegionVersionVector p_rvv = getRVV(P);
RegionVersionVector r_rvv = getRVV(R);
- out.println("GGG:p_rvv=" + p_rvv.fullToString() + ":r_rvv=" + r_rvv.fullToString());
+ out.println("p_rvv=" + p_rvv.fullToString() + ":r_rvv=" + r_rvv.fullToString());
- waitForToVerifyRVV(R, memberP, 7, null, 4); // R's rvv=p7, gc=4
- waitForToVerifyRVV(R, memberR, 6, null, 5); // R's rvv=r6, gc=5
+ waitForToVerifyRVV(R, memberP, 7, null, 0); // R's rvv=p7, gc=0
+ waitForToVerifyRVV(R, memberR, 6, null, 0); // R's rvv=r6, gc=0
waitForToVerifyRVV(P, memberP, 7, null, 0); // P's rvv=p7, gc=0
waitForToVerifyRVV(P, memberR, 6, null, 0); // P's rvv=r6, gc=0
}
@@ -2704,7 +2704,6 @@ public class GIIDeltaDUnitTest extends JUnit4CacheTestCase {
assertTrue(entry != null && entry.getRegionEntry().isTombstone());
}
- System.out.println("GGG:new timeout=" + TombstoneService.REPLICATE_TOMBSTONE_TIMEOUT);
if (entry == null || !entry.getRegionEntry().isTombstone()) {
return (false == expectExist);
} else {
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java b/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
index 416e20a..6ca640f 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
@@ -373,7 +373,7 @@ public class TombstoneService {
return this.replicatedTombstoneSweeper.getBlockGCLock();
}
- private static class Tombstone extends CompactVersionHolder {
+ protected static class Tombstone extends CompactVersionHolder {
// tombstone overhead size
public static final int PER_TOMBSTONE_OVERHEAD =
ReflectionSingleObjectSizer.REFERENCE_SIZE // queue's reference to the tombstone
@@ -452,7 +452,7 @@ public class TombstoneService {
protected void beforeSleepChecks() {}
}
- private static class ReplicateTombstoneSweeper extends TombstoneSweeper {
+ protected static class ReplicateTombstoneSweeper extends TombstoneSweeper {
/**
* Used to execute batch gc message execution in the background.
*/
@@ -540,7 +540,8 @@ public class TombstoneService {
}
/** expire a batch of tombstones */
- private void expireBatch() {
+
+ protected void expireBatch() {
// fix for bug #46087 - OOME due to too many GC threads
if (this.batchExpirationInProgress) {
// incorrect return due to race between this and waiting-pool GC thread is okay
@@ -575,6 +576,9 @@ public class TombstoneService {
synchronized (expiredTombstonesLock) {
for (Tombstone t : expiredTombstones) {
DistributedRegion tr = (DistributedRegion) t.region;
+ if (!tr.isInitialized()) {
+ continue;
+ }
tr.getVersionVector().recordGCVersion(t.getMemberID(), t.getRegionVersion());
if (!reapedKeys.containsKey(tr)) {
reapedKeys.put(tr, Collections.emptySet());
@@ -600,15 +604,17 @@ public class TombstoneService {
// for PR buckets we have to keep track of the keys removed because clients have
// them all lumped in a single non-PR region
DistributedRegion tr = (DistributedRegion) t.region;
- boolean tombstoneWasStillInRegionMap =
- tr.getRegionMap().removeTombstone(t.entry, t, false, true);
- if (tombstoneWasStillInRegionMap && hasToTrackKeysForClients(tr)) {
- Set<Object> keys = reapedKeys.get(tr);
- if (keys.isEmpty()) {
- keys = new HashSet<Object>();
- reapedKeys.put(tr, keys);
+ if (reapedKeys.containsKey(tr)) {
+ boolean tombstoneWasStillInRegionMap =
+ tr.getRegionMap().removeTombstone(t.entry, t, false, true);
+ if (tombstoneWasStillInRegionMap && hasToTrackKeysForClients(tr)) {
+ Set<Object> keys = reapedKeys.get(tr);
+ if (keys.isEmpty()) {
+ keys = new HashSet<Object>();
+ reapedKeys.put(tr, keys);
+ }
+ keys.add(t.entry.getKey());
}
- keys.add(t.entry.getKey());
}
return true;
});
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java
new file mode 100644
index 0000000..606738c
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.concurrent.ExecutorService;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.DataPolicy;
+import org.apache.geode.distributed.internal.CacheTime;
+import org.apache.geode.internal.cache.versions.RegionVersionVector;
+import org.apache.geode.internal.cache.versions.VersionTag;
+
+public class TombstoneServiceTest {
+ CacheTime cacheTime;
+ CachePerfStats stats;
+ CancelCriterion cancelCriterion;
+ ExecutorService executor;
+ RegionMap regionMap;
+ RegionEntry entry;
+ DistributedRegion region;
+ VersionTag destroyedVersion;
+ private TombstoneService.ReplicateTombstoneSweeper replicateTombstoneSweeper;
+ private TombstoneService.Tombstone tombstone;
+
+
+ @Before
+ public void setUp() throws Exception {
+ cacheTime = mock(CacheTime.class);
+ stats = mock(CachePerfStats.class);
+ cancelCriterion = mock(CancelCriterion.class);
+ executor = mock(ExecutorService.class);
+ regionMap = mock(RegionMap.class);
+ entry = mock(RegionEntry.class);
+ region = mock(DistributedRegion.class);
+ destroyedVersion = mock(VersionTag.class);
+ replicateTombstoneSweeper = new TombstoneService.ReplicateTombstoneSweeper(cacheTime, stats,
+ cancelCriterion, executor);
+ tombstone = new TombstoneService.Tombstone(entry, region, destroyedVersion);
+ tombstone.entry = entry;
+ }
+
+ @Test
+ public void validateThatRemoveIsNotCalledOnTombstoneInRegionThatIsNotInitialized() {
+ when(region.isInitialized()).thenReturn(false);
+ when(region.getRegionMap()).thenReturn(regionMap);
+
+ replicateTombstoneSweeper.expireTombstone(tombstone);
+ replicateTombstoneSweeper.expireBatch();
+ verify(regionMap, Mockito.never()).removeTombstone(tombstone.entry, tombstone, false,
+ true);
+ }
+
+ @Test
+ public void validateThatRemoveIsCalledOnTombstoneInRegionThatIsInitialized() {
+ RegionVersionVector regionVersionVector = mock(RegionVersionVector.class);
+
+ when(region.isInitialized()).thenReturn(true);
+ when(region.getRegionMap()).thenReturn(regionMap);
+ when(region.getVersionVector()).thenReturn(regionVersionVector);
+ when(region.getDataPolicy()).thenReturn(DataPolicy.PERSISTENT_REPLICATE);
+ when(region.getDiskRegion()).thenReturn(mock(DiskRegion.class));
+
+
+ replicateTombstoneSweeper.expireTombstone(tombstone);
+ replicateTombstoneSweeper.expireBatch();
+ verify(regionMap, Mockito.times(1)).removeTombstone(tombstone.entry, tombstone, false,
+ true);
+ }
+}