[GitHub] [geode] DonalEvans commented on a change in pull request #6042: GEODE-8958: When timestamps get corrupted.

[GitBox <gi...@apache.org>]

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