From de376be2ba3c9f20de5eecfdaae2bb028893a17a Mon Sep 17 00:00:00 2001 From: Hui Xiao Date: Fri, 6 Jun 2025 12:40:45 -0700 Subject: [PATCH] Simplify RoundRobinSubcompactionsAgainstResources.SubcompactionsUsingResources test (#13672) Summary: **Context/Summary:** `RoundRobinSubcompactionsAgainstResources.SubcompactionsUsingResources` has been flaky and difficult to de-flake. One of the reasons is the complicated usage of sync points and unnecessarily strict verification. - The sync points don't seem necessary to verify the number of extra reserved threads for sub-compactions so are removed. - The full reservation after compaction to verify extra reserved threads were release is indirect and hard to get right. So it's replaced with simpler sync-point callback check. - Since we already have tests (see https://github.com/facebook/rocksdb/blob/7d80ea45442e84c25669db61cb7376ba0cd10ba5/env/env_test.cc#L841 and )for testing pure functionality of reserve/release does reserve/release the threads, verifying the relevant code paths are called should be enough to verify extra reserved threads were released after compaction Pull Request resolved: https://github.com/facebook/rocksdb/pull/13672 Test Plan: Monitor future flakiness. Reviewed By: cbi42 Differential Revision: D76108242 Pulled By: hx235 fbshipit-source-id: 30113f16455688f113f296bda0098a66a7a198a3 --- db/compaction/compaction_job.cc | 3 +-- db/db_compaction_test.cc | 48 ++++++++++++++------------------- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index 1f521f7d2d..f2a36907de 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -839,8 +839,7 @@ Status CompactionJob::Run() { } ReleaseSubcompactionResources(); - TEST_SYNC_POINT("CompactionJob::ReleaseSubcompactionResources:0"); - TEST_SYNC_POINT("CompactionJob::ReleaseSubcompactionResources:1"); + TEST_SYNC_POINT("CompactionJob::ReleaseSubcompactionResources"); for (const auto& state : compact_->sub_compact_states) { for (const auto& output : state.GetOutputs()) { diff --git a/db/db_compaction_test.cc b/db/db_compaction_test.cc index 98536960c8..ba4edeffa2 100644 --- a/db/db_compaction_test.cc +++ b/db/db_compaction_test.cc @@ -6634,7 +6634,7 @@ TEST_P(RoundRobinSubcompactionsAgainstResources, SubcompactionsUsingResources) { // compaction is enough to make post-compaction L1 size less than // the maximum size (this test assumes only one round-robin compaction // is triggered by kLevelMaxLevelSize) - options.max_compaction_bytes = 100000000; + options.max_compaction_bytes = std::numeric_limits::max(); DestroyAndReopen(options); env_->SetBackgroundThreads(total_low_pri_threads_, Env::LOW); @@ -6667,41 +6667,33 @@ TEST_P(RoundRobinSubcompactionsAgainstResources, SubcompactionsUsingResources) { // More than 10 files are selected for round-robin under auto // compaction. The number of planned subcompaction is restricted by // the minimum number between available threads and compaction limits - ASSERT_EQ(num_planned_subcompactions - options.max_subcompactions, - std::min(total_low_pri_threads_, max_compaction_limits_) - 1); + auto actual_reserved_threads = + num_planned_subcompactions - options.max_subcompactions; + auto expected_reserved_threads = + std::min(total_low_pri_threads_, max_compaction_limits_) - 1; + ASSERT_EQ(actual_reserved_threads, expected_reserved_threads); num_planned_subcompactions_verified = true; }); - SyncPoint::GetInstance()->LoadDependency( - {{"RoundRobinSubcompactionsAgainstResources:0", - "BackgroundCallCompaction:0"}, - {"CompactionJob::AcquireSubcompactionResources:0", - "RoundRobinSubcompactionsAgainstResources:1"}, - {"RoundRobinSubcompactionsAgainstResources:2", - "CompactionJob::AcquireSubcompactionResources:1"}, - {"CompactionJob::ReleaseSubcompactionResources:0", - "RoundRobinSubcompactionsAgainstResources:3"}, - {"RoundRobinSubcompactionsAgainstResources:4", - "CompactionJob::ReleaseSubcompactionResources:1"}}); + + int acquire_count = 0; + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( + "CompactionJob::AcquireSubcompactionResources:0", + [&](void* /*arg*/) { acquire_count++; }); + int release_count = 0; + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( + "CompactionJob::ReleaseSubcompactionResources", + [&](void* /*arg*/) { release_count++; }); + SyncPoint::GetInstance()->EnableProcessing(); - ASSERT_OK(dbfull()->TEST_WaitForCompact()); - ASSERT_OK(dbfull()->EnableAutoCompaction({dbfull()->DefaultColumnFamily()})); - TEST_SYNC_POINT("RoundRobinSubcompactionsAgainstResources:0"); - TEST_SYNC_POINT("RoundRobinSubcompactionsAgainstResources:1"); auto pressure_token = dbfull()->TEST_write_controler().GetCompactionPressureToken(); - - TEST_SYNC_POINT("RoundRobinSubcompactionsAgainstResources:2"); - TEST_SYNC_POINT("RoundRobinSubcompactionsAgainstResources:3"); - // We can reserve more threads now except one is being used - ASSERT_EQ(total_low_pri_threads_ - 1, - env_->ReserveThreads(total_low_pri_threads_, Env::Priority::LOW)); - ASSERT_EQ( - total_low_pri_threads_ - 1, - env_->ReleaseThreads(total_low_pri_threads_ - 1, Env::Priority::LOW)); - TEST_SYNC_POINT("RoundRobinSubcompactionsAgainstResources:4"); + ASSERT_OK(dbfull()->EnableAutoCompaction({dbfull()->DefaultColumnFamily()})); ASSERT_OK(dbfull()->TEST_WaitForCompact()); + ASSERT_TRUE(num_planned_subcompactions_verified); + ASSERT_EQ(acquire_count, release_count); + SyncPoint::GetInstance()->DisableProcessing(); SyncPoint::GetInstance()->ClearAllCallBacks(); }