From c184072d44559960758534050a738ad93103ffba Mon Sep 17 00:00:00 2001 From: Ben Huddleston Date: Thu, 14 Jan 2021 14:37:40 +0000 Subject: [PATCH] MB-43621: Don't count aborts towards on_disk_prepare_bytes We have a few tests that try to track on_disk_prepare_size but they used the cachedVBState which wasn't updated post flush. We shouldn't count aborts towards on_disk_prepare_bytes as we must keep them for the metadata purge interval to ensure that replicas can reconnect correctly. As such their inclusion in on_disk_prepare_bytes would potentially trigger unnecessary compactions. Change-Id: Ie42186c538bff6a5bb33dc019e03185aba1079d9 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143425 Tested-by: Build Bot Reviewed-by: Dave Rigby --- engines/ep/src/couch-kvstore/couch-kvstore.cc | 15 +-- .../tests/module_tests/couch-kvstore_test.cc | 91 +++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/engines/ep/src/couch-kvstore/couch-kvstore.cc b/engines/ep/src/couch-kvstore/couch-kvstore.cc index 81e25425df..6e689741de 100644 --- a/engines/ep/src/couch-kvstore/couch-kvstore.cc +++ b/engines/ep/src/couch-kvstore/couch-kvstore.cc @@ -2656,26 +2656,29 @@ static void saveDocsCallback(const DocInfo* oldInfo, onDiskMutationType = DocMutationType::Insert; } + const ssize_t newSize = newInfo->physical_size; + const ssize_t oldSize = oldInfo ? oldInfo->physical_size : 0; + switch (onDiskMutationType) { case DocMutationType::Delete: if (newKey.isPrepared()) { cbCtx->onDiskPrepareDelta--; + cbCtx->onDiskPrepareBytesDelta -= oldSize; } break; case DocMutationType::Insert: if (newKey.isPrepared()) { cbCtx->onDiskPrepareDelta++; + cbCtx->onDiskPrepareBytesDelta += newSize; } break; case DocMutationType::Update: + if (!newInfo->deleted) { + // Not an abort, update the stat + cbCtx->onDiskPrepareBytesDelta += (newSize - oldSize); + } break; } - - if (newKey.isPrepared()) { - const ssize_t newSize = newInfo->physical_size; - const ssize_t oldSize = oldInfo ? oldInfo->physical_size : 0; - cbCtx->onDiskPrepareBytesDelta += (newSize - oldSize); - } } /** diff --git a/engines/ep/tests/module_tests/couch-kvstore_test.cc b/engines/ep/tests/module_tests/couch-kvstore_test.cc index f35a30ddbf..be7ed1765b 100644 --- a/engines/ep/tests/module_tests/couch-kvstore_test.cc +++ b/engines/ep/tests/module_tests/couch-kvstore_test.cc @@ -1298,6 +1298,27 @@ class CouchstoreTest : public ::testing::Test { flush.proposedVBState.transition.state = vbucket_state_active; } + void persistPrepare(std::string key, std::string value, uint64_t seqno) { + auto storedDocKey = makeStoredDocKey(key); + auto qi = makePendingItem(storedDocKey, value); + qi->setBySeqno(seqno); + + kvstore->begin(std::make_unique(vbid)); + kvstore->del(qi); + kvstore->commit(flush); + } + + void persistAbort(std::string key, std::string value, uint64_t seqno) { + auto storedDocKey = makeStoredDocKey(key); + auto qi = makePendingItem(storedDocKey, value); + qi->setBySeqno(seqno); + qi->setAbortSyncWrite(); + + kvstore->begin(std::make_unique(vbid)); + kvstore->del(qi); + kvstore->commit(flush); + } + ~CouchstoreTest() override { cb::io::rmrf(data_dir.c_str()); } @@ -2191,3 +2212,73 @@ TEST_F(CouchstoreTest, ConcurrentCompactionAndFlushingAbortToAbort) { vbstate = kvstore->getPersistedVBucketState(vbid); EXPECT_EQ(0, vbstate.onDiskPrepares); } + +TEST_F(CouchstoreTest, PersistPrepareStats) { + persistPrepare("key", "value", 1); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(1, persistedVBState.onDiskPrepares); + EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_LT(0, dbFileInfo.prepareBytes); +} + +TEST_F(CouchstoreTest, PersistAbortStats) { + persistAbort("key", "value", 1); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(0, persistedVBState.onDiskPrepares); + EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_EQ(0, dbFileInfo.prepareBytes); +} + +TEST_F(CouchstoreTest, PersistPreparePrepareStats) { + persistPrepare("key", "value", 1); + persistPrepare("key", "longervalue", 2); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(1, persistedVBState.onDiskPrepares); + EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_LT(0, dbFileInfo.prepareBytes); +} + +TEST_F(CouchstoreTest, PersistPrepareAbortStats) { + persistPrepare("key", "value", 1); + persistAbort("key", "differentvalue", 2); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(0, persistedVBState.onDiskPrepares); + EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_EQ(0, dbFileInfo.prepareBytes); +} + +TEST_F(CouchstoreTest, PersistAbortPrepareStats) { + persistAbort("key", "value", 1); + persistPrepare("key", "differentvalue", 2); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(1, persistedVBState.onDiskPrepares); + EXPECT_LT(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_LT(0, dbFileInfo.prepareBytes); +} + +TEST_F(CouchstoreTest, PersistAbortAbortStats) { + persistAbort("key", "value", 1); + persistAbort("key", "differentvalue", 2); + + auto persistedVBState = kvstore->getPersistedVBucketState(vbid); + EXPECT_EQ(0, persistedVBState.onDiskPrepares); + EXPECT_EQ(0, persistedVBState.getOnDiskPrepareBytes()); + + auto dbFileInfo = kvstore->getDbFileInfo(vbid); + EXPECT_EQ(0, dbFileInfo.prepareBytes); +}