[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2641: Add checksum validation for SST files

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

eolivelli commented on a change in pull request #2641:
URL: https://github.com/apache/bookkeeper/pull/2641#discussion_r591165841



##########
File path: stream/statelib/src/test/java/org/apache/bookkeeper/statelib/impl/rocksdb/checkpoint/RocksCheckpointerTest.java
##########
@@ -349,6 +347,65 @@ public void testCheckpointOrder() throws Exception {
         }
     }
 
+    @Test
+    public void testStaleSSTFile() throws Exception {
+        final int numKvs = 100;
+        TestStateStore testStore = new TestStateStore(
+            runtime.getMethodName(), localDir, remoteDir, true, false);
+
+        store.close();
+
+        testStore.enableCheckpoints(true);
+        testStore.init();
+
+        testStore.addNumKVs("transaction-1", numKvs, 0);
+        // create base checkpoint
+        String baseCheckpoint = testStore.checkpoint("checkpoint-1");
+        testStore.restore();
+
+        testStore.addNumKVs("transaction-2", numKvs, 100);
+        // create failed checkpoint
+        String failedCheckpoint = testStore.checkpoint("checkpoint-2");
+        // Remove metadata from the checkpoint to signal failure
+
+        CheckpointInfo checkpoint = testStore.getLatestCheckpoint();
+        testStore.corruptCheckpoint(checkpoint);
+
+        // restore : this should restore from base checkpoint
+        testStore.destroyLocal();
+        testStore.restore();
+        assertEquals("transaction-1", testStore.get("transaction-id"));
+
+        testStore.addNumKVs("transaction-3", numKvs * 3, 200);
+        // create another test checkpoint
+        String newCheckpoint = testStore.checkpoint("checkpoint-3");
+
+        // Ensure latest checkpoint can be restored.
+        testStore.destroyLocal();
+        testStore.restore();
+        assertEquals("transaction-3", testStore.get("transaction-id"));
+    }
+
+    @Test
+    public void testRestoreOldCheckpointWithoutChecksum() throws Exception {

Review comment:
       +1
   yes, please add a compatibility test,
   also can you please state in the description how to enable this feature ?
   
   would it be possible to enable this feature by default and deal with old SST files that are missing the checksum ?
   otherwise I bet that no one will use this feature




----------------------------------------------------------------
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