[
https://issues.apache.org/jira/browse/GEODE-8422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17176542#comment-17176542
]
ASF GitHub Bot commented on GEODE-8422:
---------------------------------------
DonalEvans commented on a change in pull request #5447:
URL: https://github.com/apache/geode/pull/5447#discussion_r469471781
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java
##########
@@ -0,0 +1,79 @@
+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.After;
+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 = mock(CacheTime.class);
+ CachePerfStats stats = mock(CachePerfStats.class);
+ CancelCriterion cancelCriterion = mock(CancelCriterion.class);
+ ExecutorService executor = mock(ExecutorService.class);
+ RegionMap regionMap = mock(RegionMap.class);
+ RegionEntry entry = mock(RegionEntry.class);
+ DistributedRegion region = mock(DistributedRegion.class);
+ VersionTag destroyedVersion = mock(VersionTag.class);
+
+ @Before
+ public void setUp() throws Exception {}
+
+ @After
+ public void tearDown() throws Exception {}
Review comment:
This method is empty and can be removed.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java
##########
@@ -0,0 +1,79 @@
+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.After;
+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 = mock(CacheTime.class);
+ CachePerfStats stats = mock(CachePerfStats.class);
+ CancelCriterion cancelCriterion = mock(CancelCriterion.class);
+ ExecutorService executor = mock(ExecutorService.class);
+ RegionMap regionMap = mock(RegionMap.class);
+ RegionEntry entry = mock(RegionEntry.class);
+ DistributedRegion region = mock(DistributedRegion.class);
+ VersionTag destroyedVersion = mock(VersionTag.class);
+
+ @Before
+ public void setUp() throws Exception {}
+
+ @After
+ public void tearDown() throws Exception {}
+
+ @Test
+ public void
validateThatRemoveIsNotCalledOnTombstoneInRegionThatIsNotInitialized() {
+
+ when(region.isInitialized()).thenReturn(false);
+ when(region.getRegionMap()).thenReturn(regionMap);
+ TombstoneService.ReplicateTombstoneSweeper replicateTombstoneSweeper =
+ new TombstoneService.ReplicateTombstoneSweeper(cacheTime, stats,
+ cancelCriterion, executor);
+ TombstoneService.Tombstone tombstone =
+ new TombstoneService.Tombstone(entry, region, destroyedVersion);
+ tombstone.entry = entry;
+
+ replicateTombstoneSweeper.expireTombstone(tombstone);
+ replicateTombstoneSweeper.expireBatch();
+ verify(regionMap, Mockito.never()).removeTombstone(tombstone.entry,
destroyedVersion, 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));
+
+ TombstoneService.ReplicateTombstoneSweeper replicateTombstoneSweeper =
+ new TombstoneService.ReplicateTombstoneSweeper(cacheTime, stats,
+ cancelCriterion, executor);
+ TombstoneService.Tombstone tombstone =
+ new TombstoneService.Tombstone(entry, region, destroyedVersion);
+ tombstone.entry = entry;
+
+ replicateTombstoneSweeper.expireTombstone(tombstone);
+ replicateTombstoneSweeper.expireBatch();
+ verify(region, Mockito.atLeastOnce()).getVersionVector();
+ verify(regionVersionVector,
Mockito.atLeastOnce()).recordGCVersion(tombstone.getMemberID(),
Review comment:
For these verifications, would it be reasonable to assert the specific
number of invocations of `getVersionVector()` and `recordGCVersion()` we expect
to see using `Mockito.times(1)` rather than just that it's at least one?
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/TombstoneServiceTest.java
##########
@@ -0,0 +1,79 @@
+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.After;
+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 = mock(CacheTime.class);
+ CachePerfStats stats = mock(CachePerfStats.class);
+ CancelCriterion cancelCriterion = mock(CancelCriterion.class);
+ ExecutorService executor = mock(ExecutorService.class);
+ RegionMap regionMap = mock(RegionMap.class);
+ RegionEntry entry = mock(RegionEntry.class);
+ DistributedRegion region = mock(DistributedRegion.class);
+ VersionTag destroyedVersion = mock(VersionTag.class);
+
+ @Before
+ public void setUp() throws Exception {}
Review comment:
The creation and assignment of the mocks should be moved to the
`setUp()` method to ensure that fresh mocks are created for each test method
and hence prevent multiple tests acting on the same instance of a mock and
producing unexpected results.
----------------------------------------------------------------
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]
> Regions get out of sync if destroys happening during shutdown
> -------------------------------------------------------------
>
> Key: GEODE-8422
> URL: https://issues.apache.org/jira/browse/GEODE-8422
> Project: Geode
> Issue Type: Bug
> Components: general
> Affects Versions: 1.13.0, 1.14.0
> Reporter: Mark Hanson
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.13.0, 1.14.0
>
>
> When a cluster of servers are being shutdown, while destroy operations are
> happening, in a small case, it is possible for the regions to get out of sync
> when delta GII is used.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
