Make DBCompactionTest.SkipStatsUpdateTest more robust (#6306)

Summary:
Currently, this test case tries to infer whether
`VersionStorageInfo::UpdateAccumulatedStats` was called during open by
checking the number of files opened against an arbitrary threshold (10).
This makes the test brittle and results in sporadic failures. The patch
changes the test case to use sync points to directly test whether
`UpdateAccumulatedStats` was called.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6306

Test Plan: `make check`

Differential Revision: D19439544

Pulled By: ltamasi

fbshipit-source-id: ceb7adf578222636a0f51740872d0278cd1a914f
main
Levi Tamasi 5 years ago committed by Facebook Github Bot
parent 8e309b35bb
commit d305f13e21
  1. 29
      db/db_compaction_test.cc
  2. 3
      db/version_set.cc

@ -400,6 +400,7 @@ TEST_F(DBCompactionTest, SkipStatsUpdateTest) {
// The test will need to be updated if the internal behavior changes. // The test will need to be updated if the internal behavior changes.
Options options = DeletionTriggerOptions(CurrentOptions()); Options options = DeletionTriggerOptions(CurrentOptions());
options.disable_auto_compactions = true;
options.env = env_; options.env = env_;
DestroyAndReopen(options); DestroyAndReopen(options);
Random rnd(301); Random rnd(301);
@ -410,31 +411,33 @@ TEST_F(DBCompactionTest, SkipStatsUpdateTest) {
values.push_back(RandomString(&rnd, kCDTValueSize)); values.push_back(RandomString(&rnd, kCDTValueSize));
ASSERT_OK(Put(Key(k), values[k])); ASSERT_OK(Put(Key(k), values[k]));
} }
dbfull()->TEST_WaitForFlushMemTable();
dbfull()->TEST_WaitForCompact(); ASSERT_OK(Flush());
Close();
int update_acc_stats_called = 0;
rocksdb::SyncPoint::GetInstance()->SetCallBack(
"VersionStorageInfo::UpdateAccumulatedStats",
[&](void* /* arg */) { ++update_acc_stats_called; });
SyncPoint::GetInstance()->EnableProcessing();
// Reopen the DB with stats-update disabled // Reopen the DB with stats-update disabled
options.skip_stats_update_on_db_open = true; options.skip_stats_update_on_db_open = true;
options.max_open_files = 20; options.max_open_files = 20;
env_->random_file_open_counter_.store(0);
Reopen(options); Reopen(options);
// As stats-update is disabled, we expect a very low number of ASSERT_EQ(update_acc_stats_called, 0);
// random file open.
// Note that this number must be changed accordingly if we change
// the number of files needed to be opened in the DB::Open process.
const int kMaxFileOpenCount = 10;
ASSERT_LT(env_->random_file_open_counter_.load(), kMaxFileOpenCount);
// Repeat the reopen process, but this time we enable // Repeat the reopen process, but this time we enable
// stats-update. // stats-update.
options.skip_stats_update_on_db_open = false; options.skip_stats_update_on_db_open = false;
env_->random_file_open_counter_.store(0);
Reopen(options); Reopen(options);
// Since we do a normal stats update on db-open, there ASSERT_GT(update_acc_stats_called, 0);
// will be more random open files.
ASSERT_GT(env_->random_file_open_counter_.load(), kMaxFileOpenCount); SyncPoint::GetInstance()->ClearAllCallBacks();
SyncPoint::GetInstance()->DisableProcessing();
} }
TEST_F(DBCompactionTest, TestTableReaderForCompaction) { TEST_F(DBCompactionTest, TestTableReaderForCompaction) {

@ -2112,6 +2112,9 @@ bool Version::MaybeInitializeFileMetaData(FileMetaData* file_meta) {
} }
void VersionStorageInfo::UpdateAccumulatedStats(FileMetaData* file_meta) { void VersionStorageInfo::UpdateAccumulatedStats(FileMetaData* file_meta) {
TEST_SYNC_POINT_CALLBACK("VersionStorageInfo::UpdateAccumulatedStats",
nullptr);
assert(file_meta->init_stats_from_file); assert(file_meta->init_stats_from_file);
accumulated_file_size_ += file_meta->fd.GetFileSize(); accumulated_file_size_ += file_meta->fd.GetFileSize();
accumulated_raw_key_size_ += file_meta->raw_key_size; accumulated_raw_key_size_ += file_meta->raw_key_size;

Loading…
Cancel
Save