[
https://issues.apache.org/jira/browse/GEODE-8958?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17289401#comment-17289401
]
ASF GitHub Bot commented on GEODE-8958:
---------------------------------------
jchen21 commented on a change in pull request #6042:
URL: https://github.com/apache/geode/pull/6042#discussion_r581426189
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -117,35 +118,74 @@ public void
testTombstoneGcMessagesBetweenPersistentAndNonPersistentRegion() {
});
}
+
@Test
- public void testGetOldestTombstoneTimeReplicate() {
+ public void
testWhenAnOutOfRangeTimeStampIsSeenWeExpireItInReplicateTombstoneSweeper() {
VM server1 = VM.getVM(0);
VM server2 = VM.getVM(1);
+ final int FAR_INTO_THE_FUTURE = 1000000; // 1 million millis into the
future
+ final int count = 10;
server1.invoke(() -> {
- createCacheAndRegion(REPLICATE_PERSISTENT);
- region.put("K1", "V1");
- region.put("K2", "V2");
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
});
- server2.invoke(() -> createCacheAndRegion(REPLICATE));
+ 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() +
FAR_INTO_THE_FUTURE);
+
+ TombstoneService.Tombstone modifiedTombstone =
+ new TombstoneService.Tombstone(regionEntry, (LocalRegion) region,
+ versionTag);
+ tombstoneSweeper.tombstones.add(modifiedTombstone);
+ tombstoneSweeper.checkOldestUnexpired(System.currentTimeMillis());
// Send tombstone gc message to vm1.
Review comment:
This comment does not apply to the modified code. It is better to update
the comment accordingly.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -117,35 +118,74 @@ public void
testTombstoneGcMessagesBetweenPersistentAndNonPersistentRegion() {
});
}
+
@Test
- public void testGetOldestTombstoneTimeReplicate() {
+ public void
testWhenAnOutOfRangeTimeStampIsSeenWeExpireItInReplicateTombstoneSweeper() {
VM server1 = VM.getVM(0);
VM server2 = VM.getVM(1);
+ final int FAR_INTO_THE_FUTURE = 1000000; // 1 million millis into the
future
+ final int count = 10;
server1.invoke(() -> {
- createCacheAndRegion(REPLICATE_PERSISTENT);
- region.put("K1", "V1");
- region.put("K2", "V2");
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
});
- server2.invoke(() -> createCacheAndRegion(REPLICATE));
+ server2.invoke(() -> createCacheAndRegion(RegionShortcut.REPLICATE));
Review comment:
Can you explain a bit why the region shortcut on server2 is not the same
as that of server1 `REPLICATE_PERSISTENT` ? I understand this is not your code
change though.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/versions/TombstoneDUnitTest.java
##########
@@ -117,35 +118,74 @@ public void
testTombstoneGcMessagesBetweenPersistentAndNonPersistentRegion() {
});
}
+
@Test
- public void testGetOldestTombstoneTimeReplicate() {
+ public void
testWhenAnOutOfRangeTimeStampIsSeenWeExpireItInReplicateTombstoneSweeper() {
VM server1 = VM.getVM(0);
VM server2 = VM.getVM(1);
+ final int FAR_INTO_THE_FUTURE = 1000000; // 1 million millis into the
future
+ final int count = 10;
server1.invoke(() -> {
- createCacheAndRegion(REPLICATE_PERSISTENT);
- region.put("K1", "V1");
- region.put("K2", "V2");
+ createCacheAndRegion(RegionShortcut.REPLICATE_PERSISTENT);
+ for (int i = 0; i < count; i++) {
+ region.put("K" + i, "V" + i);
+ }
});
- server2.invoke(() -> createCacheAndRegion(REPLICATE));
+ 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() +
FAR_INTO_THE_FUTURE);
+
+ TombstoneService.Tombstone modifiedTombstone =
+ new TombstoneService.Tombstone(regionEntry, (LocalRegion) region,
+ versionTag);
+ tombstoneSweeper.tombstones.add(modifiedTombstone);
+ tombstoneSweeper.checkOldestUnexpired(System.currentTimeMillis());
// Send tombstone gc message to vm1.
- region.destroy("K1");
+ assertThat(tombstoneSweeper.getOldestTombstoneTime()).isEqualTo(0);
+ });
+ }
+ @Test
+ public void testGetOldestTombstoneTimeForReplicateTombstoneSweeper() {
+ 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(
Review comment:
I am not sure I understand the purpose of this assert. Even without the
fix, the assert will succeed.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java
##########
@@ -1078,8 +1087,8 @@ private void checkOldestUnexpired(long now) {
if (logger.isTraceEnabled(LogMarker.TOMBSTONE_VERBOSE)) {
logger.trace(LogMarker.TOMBSTONE_VERBOSE, "oldest unexpired
tombstone is {}", oldest);
}
- long msTillHeadTombstoneExpires = oldest.getVersionTimeStamp() +
EXPIRY_TIME - now;
- if (hasExpired(msTillHeadTombstoneExpires)) {
+ long msUntilHeadTombstoneExpires = oldest.getVersionTimeStamp() +
EXPIRY_TIME - now;
Review comment:
Minor pick: if you rename the variable to `msUntilHeadTombstoneExpires`,
it is better to rename the variable at other places, e.g. at line 759 as well,
in order to be consistent. It is better for code maintenance.
----------------------------------------------------------------
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)
