From 56dbcb4f72b96c74906a1b30408a045a30ca5a9a Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Fri, 5 Aug 2022 12:58:07 -0700 Subject: [PATCH] Deflake ChargeFileMetadataTestWithParam/ChargeFileMetadataTestWithParam.Basic/0 (#10481) Summary: **Context/summary:** `ChargeFileMetadataTestWithParam/ChargeFileMetadataTestWithParam.Basic/0 ` relies on `DBImpl::BackgroundCallCompaction:PurgedObsoleteFiles` happens before verifying `EXPECT_EQ(file_metadata_charge_only_cache->GetCacheCharge(), 1 * CacheReservationManagerImpl< CacheEntryRole::kFileMetadata>::GetDummyEntrySize());` or `EXPECT_EQ(file_metadata_charge_only_cache->GetCacheCharge(), 0);` to ensure appropriate cache reservation release is done before checking. However, this might not be the case under some timing delay and spurious wake-up as coerced below. ``` diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index 4378f3212..3e4f60853 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -2989,6 +2989,8 @@ void DBImpl::BackgroundCallCompaction(PrepickedCompaction* prepicked_compaction, if (job_context.HaveSomethingToClean() || job_context.HaveSomethingToDelete() || !log_buffer.IsEmpty()) { mutex_.Unlock(); + bg_cv_.SignalAll(); + usleep(1000); // Have to flush the info logs before bg_compaction_scheduled_-- // because if bg_flush_scheduled_ becomes 0 and the lock is // released, the deconstructor of DB can kick in and destroy all the // states of DB so info_log might not be available after that point. // It also applies to access other states that DB owns. log_buffer.FlushBufferToLog(); if (job_context.HaveSomethingToDelete()) { PurgeObsoleteFiles(job_context); TEST_SYNC_POINT("DBImpl::BackgroundCallCompaction:PurgedObsoleteFiles"); } ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/10481 Test Plan: The test of interest failed often at the above coercion: After fix, the test of interest passed at the above coercion: Reviewed By: jay-zhuang Differential Revision: D38438256 Pulled By: hx235 fbshipit-source-id: de80ecdb250174f00e7c2f5e4d952695ed56f51e --- db/version_set_test.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/db/version_set_test.cc b/db/version_set_test.cc index bb10017e9..410c2bc65 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -3526,9 +3526,15 @@ TEST_P(ChargeFileMetadataTestWithParam, Basic) { // file metadata above. This results in 1 * // CacheReservationManagerImpl::GetDummyEntrySize() // cache reservation for file metadata. + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::BackgroundCallCompaction:PurgedObsoleteFiles", + "ChargeFileMetadataTestWithParam::" + "PreVerifyingCacheReservationRelease"}}); + SyncPoint::GetInstance()->EnableProcessing(); ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)); ASSERT_EQ("0,1", FilesPerLevel(0)); - + TEST_SYNC_POINT( + "ChargeFileMetadataTestWithParam::PreVerifyingCacheReservationRelease"); if (charge_file_metadata == CacheEntryRoleOptions::Decision::kEnabled) { EXPECT_EQ(file_metadata_charge_only_cache->GetCacheCharge(), 1 * CacheReservationManagerImpl< @@ -3536,6 +3542,7 @@ TEST_P(ChargeFileMetadataTestWithParam, Basic) { } else { EXPECT_EQ(file_metadata_charge_only_cache->GetCacheCharge(), 0); } + SyncPoint::GetInstance()->DisableProcessing(); // Destroying the db will delete the remaining 1 new file metadata // This results in no cache reservation for file metadata.