From 9b663313880b556b77b9baa69b374bb4a35b8616 Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Mon, 6 Feb 2023 16:10:03 -0800 Subject: [PATCH] Simplify TEST_F(DBWALTest, FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL) (#11186) Summary: **Context/Summary**: Simplify `TEST_F(DBWALTest, FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL)` based on https://github.com/facebook/rocksdb/pull/11016#pullrequestreview-1205020134 and delete unused sync points. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11186 Test Plan: - UT failed before fix in https://github.com/facebook/rocksdb/pull/10892 and passes after - Check UT not flaky when running with https://app.circleci.com/pipelines/github/facebook/rocksdb/21985/workflows/5f6cc355-78c1-46d8-89ee-0fd679725a8a/jobs/540878 Reviewed By: ajkr Differential Revision: D43034723 Pulled By: hx235 fbshipit-source-id: f503774987b8f3718505f99e95080a7fad28ac66 --- db/db_impl/db_impl_files.cc | 2 -- db/db_wal_test.cc | 46 +++++++++++-------------------------- db/version_set.cc | 5 ---- 3 files changed, 14 insertions(+), 39 deletions(-) diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index b65c8732c..414c1270c 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -361,8 +361,6 @@ void DBImpl::DeleteObsoleteFileImpl(int job_id, const std::string& fname, } TEST_SYNC_POINT_CALLBACK("DBImpl::DeleteObsoleteFileImpl:AfterDeletion", &file_deletion_status); - TEST_SYNC_POINT_CALLBACK("DBImpl::DeleteObsoleteFileImpl:AfterDeletion2", - const_cast(&fname)); if (file_deletion_status.ok()) { ROCKS_LOG_DEBUG(immutable_db_options_.info_log, "[JOB %d] Delete %s type=%d #%" PRIu64 " -- %s\n", job_id, diff --git a/db/db_wal_test.cc b/db/db_wal_test.cc index e733cf106..705f53f90 100644 --- a/db/db_wal_test.cc +++ b/db/db_wal_test.cc @@ -1633,6 +1633,8 @@ TEST_F(DBWALTest, RaceInstallFlushResultsWithWalObsoletion) { TEST_F(DBWALTest, FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL) { Options options = CurrentOptions(); + // Small size to force manifest creation + options.max_manifest_file_size = 1; options.track_and_verify_wals_in_manifest = true; DestroyAndReopen(options); @@ -1649,53 +1651,33 @@ TEST_F(DBWALTest, FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL) { // (2) SyncWAL() proceeds with the lock. It // creates a new manifest and syncs all the inactive wals before the latest // (i.e, active log), which is 4.log. Note that SyncWAL() is not aware of the - // fact that 4.log has marked as to be obseleted. Prior to the fix, such wal + // fact that 4.log has marked as to be obseleted. Such wal // sync will then add a WAL addition record of 4.log to the new manifest - // without any special treatment. - // (3) BackgroundFlush() will eventually purge 4.log. + // without any special treatment. Prior to the fix, there is no WAL deletion + // record to offset it. (3) BackgroundFlush() will eventually purge 4.log. + bool wal_synced = false; SyncPoint::GetInstance()->SetCallBack( "FindObsoleteFiles::PostMutexUnlock", [&](void*) { ASSERT_OK(env_->FileExists(wal_file_path)); - - SyncPoint::GetInstance()->SetCallBack( - "VersionSet::ProcessManifestWrites:" - "PostDecidingCreateNewManifestOrNot", - [&](void* arg) { - bool* new_descriptor_log = (bool*)arg; - *new_descriptor_log = true; - }); - + uint64_t pre_sync_wal_manifest_no = + dbfull()->TEST_Current_Manifest_FileNo(); ASSERT_OK(db_->SyncWAL()); + uint64_t post_sync_wal_manifest_no = + dbfull()->TEST_Current_Manifest_FileNo(); + bool new_manifest_created = + post_sync_wal_manifest_no == pre_sync_wal_manifest_no + 1; + ASSERT_TRUE(new_manifest_created); wal_synced = true; }); - SyncPoint::GetInstance()->SetCallBack( - "DBImpl::DeleteObsoleteFileImpl:AfterDeletion2", [&](void* arg) { - std::string* file_name = (std::string*)arg; - if (*file_name == wal_file_path) { - TEST_SYNC_POINT( - "DBWALTest::" - "FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL::" - "PostDeleteWAL"); - } - }); - - SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::BackgroundCallFlush:FilesFound", - "PreConfrimObsoletedWALSynced"}, - {"DBWALTest::FixSyncWalOnObseletedWalWithNewManifestCausingMissingWAL::" - "PostDeleteWAL", - "PreConfrimWALDeleted"}}); SyncPoint::GetInstance()->EnableProcessing(); ASSERT_OK(Flush()); + ASSERT_OK(dbfull()->TEST_WaitForBackgroundWork()); - TEST_SYNC_POINT("PreConfrimObsoletedWALSynced"); ASSERT_TRUE(wal_synced); - - TEST_SYNC_POINT("PreConfrimWALDeleted"); // BackgroundFlush() purged 4.log // because the memtable associated with the WAL was flushed and new WAL was // created (i.e, 8.log) diff --git a/db/version_set.cc b/db/version_set.cc index 402e51721..fd0c8e06b 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -5077,15 +5077,10 @@ Status VersionSet::ProcessManifestWrites( if (!descriptor_log_ || manifest_file_size_ > db_options_->max_manifest_file_size) { TEST_SYNC_POINT("VersionSet::ProcessManifestWrites:BeforeNewManifest"); - TEST_SYNC_POINT_CALLBACK( - "VersionSet::ProcessManifestWrites:BeforeNewManifest", nullptr); new_descriptor_log = true; } else { pending_manifest_file_number_ = manifest_file_number_; } - TEST_SYNC_POINT_CALLBACK( - "VersionSet::ProcessManifestWrites:PostDecidingCreateNewManifestOrNot", - &new_descriptor_log); // Local cached copy of state variable(s). WriteCurrentStateToManifest() // reads its content after releasing db mutex to avoid race with