diff --git a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc index 1bbd195c84..cf1d78eb44 100644 --- a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc +++ b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc @@ -301,7 +301,6 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx, "{}", userSanitizedItemStr); } - maybeUpdatePurgeSeqno(); return true; } } diff --git a/engines/ep/tests/module_tests/magma_bucket_tests.cc b/engines/ep/tests/module_tests/magma_bucket_tests.cc index c9482a6827..16d19dd93c 100644 --- a/engines/ep/tests/module_tests/magma_bucket_tests.cc +++ b/engines/ep/tests/module_tests/magma_bucket_tests.cc @@ -34,9 +34,11 @@ class STParamMagmaBucketTest : public STParamPersistentBucketTest { * test the post-compaction state. * * @param storeItemsForTest Lambda storing items we care about testing + * @param postTimeTravelFn Labmda for doing things after the time travel */ void setupForImplicitCompactionTest( - std::function storeItemsForTest); + std::function storeItemsForTest, + std::function postTimeTravelFn = []() {}); }; /** @@ -263,7 +265,8 @@ TEST_P(STParamMagmaBucketTest, makeCompactionContextSetupAtWarmup) { } void STParamMagmaBucketTest::setupForImplicitCompactionTest( - std::function storeItemsForTest) { + std::function storeItemsForTest, + std::function postTimeTravelFn) { // Function to perform 16 writes so that we hit the LSMMaxNumLevel0Tables // threshold which will trigger implicit compaction auto performWritesForImplicitCompaction = [this]() -> void { @@ -302,6 +305,8 @@ void STParamMagmaBucketTest::setupForImplicitCompactionTest( // compact TimeTraveller timmy{60 * 60 * 24 * 5}; + postTimeTravelFn(); + ThreadGate tg(2); auto& bucket = dynamic_cast(*store); bucket.postPurgeSeqnoImplicitCompactionHook = [&tg]() -> void { @@ -407,6 +412,45 @@ TEST_P(STParamMagmaBucketTest, EXPECT_EQ(expectedPurgeSeqno, vb->getPurgeSeqno()); } +TEST_P(STParamMagmaBucketTest, + CheckImplicitCompactionDoesNotUpdatePurgeSeqnoForLogicallyDeletedItem) { + uint64_t expectedPurgeSeqno; + auto purgedKey = makeStoredDocKey("keyA", CollectionEntry::fruit.getId()); + CollectionsManifest cm; + + setupForImplicitCompactionTest( + [this, &expectedPurgeSeqno, &purgedKey, &cm]() { + auto vb = store->getVBucket(vbid); + expectedPurgeSeqno = vb->getHighSeqno(); + + cm.add(CollectionEntry::fruit); + vb->updateFromManifest(makeManifest(cm)); + store_item(vbid, purgedKey, "value"); + flushVBucketToDiskIfPersistent(vbid, 2); + }, + [this, &cm]() { + // Now remove the collection (which won't get purged as this is + // run after the time travel. This can't be purged or it moves + // the purge seqno and invalidates the test + auto vb = store->getVBucket(vbid); + cm.remove(CollectionEntry::fruit); + vb->updateFromManifest(makeManifest(cm)); + flushVBucketToDiskIfPersistent(vbid, 1); + }); + + auto magmaKVStore = + dynamic_cast(store->getRWUnderlying(vbid)); + ASSERT_TRUE(magmaKVStore); + // Assert that the collection key no longer has a tomb stone + auto gv = magmaKVStore->get(DiskDocKey(purgedKey), vbid); + ASSERT_EQ(cb::engine_errc::no_such_key, gv.getStatus()); + + // Ensure that the purge seqno has been set during the second flush to where + // the first tombstone was + auto vb = store->getVBucket(vbid); + EXPECT_EQ(expectedPurgeSeqno, vb->getPurgeSeqno()); +} + /** * Test that implicit/explicit compaction overlapping handles setting the purge * seqno correctly.