[
https://issues.apache.org/jira/browse/GEODE-8958?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17288593#comment-17288593
]
ASF GitHub Bot commented on GEODE-8958:
---------------------------------------
DonalEvans commented on a change in pull request #6042:
URL: https://github.com/apache/geode/pull/6042#discussion_r580518471
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
##########
@@ -373,7 +373,8 @@ public Object getBlockGCLock() {
return this.replicatedTombstoneSweeper.getBlockGCLock();
}
- protected static class Tombstone extends CompactVersionHolder {
+ @VisibleForTesting
+ public static class Tombstone extends CompactVersionHolder {
Review comment:
To prevent having to make this class and its fields/methods public,
would it be possible to move TombstoneDUnitTest to the same package as this
one? Or to move this class to the same package as TeombstoneDUnitTest?
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -144,6 +145,69 @@ public void testGetOldestTombstoneTimeReplicate() {
});
}
+ @Test
+ public void testRewriteBadOldestTombstoneTimeReplicateForTimestamp() {
+ VM server1 = VM.getVM(0);
+ VM server2 = VM.getVM(1);
+
+ final int count = 10;
+ server1.invoke(() -> {
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
+ });
+
+ server2.invoke(() -> createCacheAndRegion(RegionShortcut.REPLICATE));
+
+ server1.invoke(() -> {
+ TombstoneService.TombstoneSweeper tombstoneSweeper =
+ ((InternalCache)
cache).getTombstoneService().getSweeper((LocalRegion) region);
+
+ RegionEntry regionEntry = ((LocalRegion) region).getRegionEntry("K0");
+ VersionTag<?> versionTag = regionEntry.getVersionStamp().asVersionTag();
+ versionTag.setVersionTimeStamp(System.currentTimeMillis() + 1000000);
+
+ TombstoneService.Tombstone modifiedTombstone =
+ new TombstoneService.Tombstone(regionEntry, (LocalRegion) region,
+ versionTag);
+ tombstoneSweeper.tombstones.add(modifiedTombstone);
+ tombstoneSweeper.checkOldestUnexpired(System.currentTimeMillis());
+ // Send tombstone gc message to vm1.
+ assertThat(tombstoneSweeper.getOldestTombstoneTime()).isEqualTo(0);
+ });
+ }
+
+
+ @Test
+ public void testGetOldestTombstoneTimeReplicateForTimestamp() {
+ VM server1 = VM.getVM(0);
+ VM server2 = VM.getVM(1);
+ final int count = 10;
+ server1.invoke(() -> {
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
+ });
+
+ server2.invoke(() -> createCacheAndRegion(RegionShortcut.REPLICATE));
+
+ server1.invoke(() -> {
+ TombstoneService.TombstoneSweeper tombstoneSweeper =
+ ((InternalCache)
cache).getTombstoneService().getSweeper((LocalRegion) region);
+ // Send tombstone gc message to vm1.
+ for (int i = 0; i < count; i++) {
+ region.destroy("K" + i);
+ assertThat(tombstoneSweeper.getOldestTombstoneTime() + 30000 -
System.currentTimeMillis())
Review comment:
Where is this value of 30000 coming from? Would it be possible to
replace it with a reference to a constant rather than hard-coding it?
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -144,6 +145,69 @@ public void testGetOldestTombstoneTimeReplicate() {
});
}
+ @Test
+ public void testRewriteBadOldestTombstoneTimeReplicateForTimestamp() {
Review comment:
This test name is a little unclear. Would it be possible to rename the
test to make it clear what behaviour is being tested and what the expected
outcome is?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
##########
@@ -749,7 +751,8 @@ protected boolean hasExpired(long
msTillHeadTombstoneExpires) {
testHook_forceExpirationCount--;
return true;
}
- return msTillHeadTombstoneExpires <= 0;
+ // In case
+ return msTillHeadTombstoneExpires <= 0 || msTillHeadTombstoneExpires >
EXPIRY_TIME;
Review comment:
It seems like this fix is just masking the real issue, which is that
timestamps shouldn't be far in the future in the first place. Do we have any
idea why/how that might be happening?
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -144,6 +145,69 @@ public void testGetOldestTombstoneTimeReplicate() {
});
}
+ @Test
+ public void testRewriteBadOldestTombstoneTimeReplicateForTimestamp() {
+ VM server1 = VM.getVM(0);
+ VM server2 = VM.getVM(1);
+
+ final int count = 10;
+ server1.invoke(() -> {
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
+ });
+
+ server2.invoke(() -> createCacheAndRegion(RegionShortcut.REPLICATE));
+
+ server1.invoke(() -> {
+ TombstoneService.TombstoneSweeper tombstoneSweeper =
+ ((InternalCache)
cache).getTombstoneService().getSweeper((LocalRegion) region);
+
+ RegionEntry regionEntry = ((LocalRegion) region).getRegionEntry("K0");
+ VersionTag<?> versionTag = regionEntry.getVersionStamp().asVersionTag();
+ versionTag.setVersionTimeStamp(System.currentTimeMillis() + 1000000);
+
+ TombstoneService.Tombstone modifiedTombstone =
+ new TombstoneService.Tombstone(regionEntry, (LocalRegion) region,
+ versionTag);
+ tombstoneSweeper.tombstones.add(modifiedTombstone);
+ tombstoneSweeper.checkOldestUnexpired(System.currentTimeMillis());
+ // Send tombstone gc message to vm1.
+ assertThat(tombstoneSweeper.getOldestTombstoneTime()).isEqualTo(0);
+ });
+ }
+
+
+ @Test
+ public void testGetOldestTombstoneTimeReplicateForTimestamp() {
Review comment:
This test name is also a little unclear in what the test is actually
doing. Would it be possible to change it to something that conveys what the
behaviour being tested is and what the expected outcome is?
----------------------------------------------------------------
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
> Tombstone expiration: in the event that a version timestamp is too far in the
> future, expire the tombstone
> ----------------------------------------------------------------------------------------------------------
>
> Key: GEODE-8958
> URL: https://issues.apache.org/jira/browse/GEODE-8958
> Project: Geode
> Issue Type: Bug
> Components: core
> Reporter: Mark Hanson
> Assignee: Mark Hanson
> Priority: Major
> Labels: pull-request-available
>
> We are seeing a bug where for some reason, the version timestamp on the
> tombstone is too far into the future to be realistic.
>
> In such a case, we are going to expire the tombstone as soon as we see it.
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
