Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move dump stats to a separate thread #4382

Closed
wants to merge 9 commits into from
Closed

move dump stats to a separate thread #4382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3105c30
move dump stats in a separate thread
miasantreble Sep 17, 2018
fe887bc
add cast to avoid overflow
miasantreble Sep 19, 2018
6e90ec4
use RepeatableThread to schedule timed task
miasantreble Sep 28, 2018
08d7efc
fix hang & address review comments
miasantreble Oct 3, 2018
c071d5b
fix deadlock
miasantreble Oct 4, 2018
6f4436f
address Yi's comments
miasantreble Oct 7, 2018
7236bf5
add test coverage
miasantreble Oct 8, 2018
7ad5d45
address review comments
miasantreble Oct 8, 2018
7fa6a49
fix travis crash
miasantreble Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion db/db_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ void DBImpl::StartTimedTasks() {
}

void DBImpl::DumpStats() {
TEST_SYNC_POINT("DBImpl::DumpStats:1");
#ifndef ROCKSDB_LITE
const DBPropertyInfo* cf_property_info =
GetPropertyInfo(DB::Properties::kCFStats);
Expand Down Expand Up @@ -631,6 +632,7 @@ void DBImpl::DumpStats() {
}
}
}
TEST_SYNC_POINT("DBImpl::DumpStats:2");
ROCKS_LOG_WARN(immutable_db_options_.info_log,
"------- DUMPING STATS -------");
ROCKS_LOG_WARN(immutable_db_options_.info_log, "%s", stats.c_str());
Expand Down Expand Up @@ -771,7 +773,15 @@ Status DBImpl::SetDBOptions(
new_options.max_background_compactions, Env::Priority::LOW);
MaybeScheduleFlushOrCompaction();
}
if (new_options.stats_dump_period_sec > 0 &&
if (new_options.stats_dump_period_sec == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The logic can be simplified as

if (new_options.xxx != existing_options.xxx) {
  if (thread_dump_stats_ != nullptr) {
    cancel_thread();
  }
  if (new_options.xxx > 0) {
    start_thread();
  }
}

if (thread_dump_stats_) {
mutex_.Unlock();
thread_dump_stats_->cancel();
mutex_.Lock();
thread_dump_stats_.reset();
}
}
else if (new_options.stats_dump_period_sec > 0 &&
new_options.stats_dump_period_sec !=
mutable_db_options_.stats_dump_period_sec) {
if (thread_dump_stats_) {
Expand Down
24 changes: 24 additions & 0 deletions db/db_options_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,30 @@ TEST_F(DBOptionsTest, SetStatsDumpPeriodSec) {
}
}

TEST_F(DBOptionsTest, RunStatsDumpPeriodSec) {
Options options;
options.create_if_missing = true;
options.stats_dump_period_sec = 5;
options.env = env_;
int counter = 0;
rocksdb::SyncPoint::GetInstance()->SetCallBack(
"DBImpl::DumpStats:1", [&](void* /*arg*/) {
counter++;
});
rocksdb::SyncPoint::GetInstance()->EnableProcessing();
Reopen(options);
ASSERT_EQ(5, dbfull()->GetDBOptions().stats_dump_period_sec);
env_->SleepForMicroseconds(6000000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be flaky to rely on sleep. Do you consider either use a cond var like the RepeatableThreadTest::TimedTest, or use MockTimeEnv like RepeatableThreadTest::MockEnvTest to make the test more deterministic? :)
https://github.com/facebook/rocksdb/blob/master/util/repeatable_thread_test.cc#L22

ASSERT_GE(counter, 1);

// Test cacel job through SetOptions
ASSERT_OK(dbfull()->SetDBOptions(
{{"stats_dump_period_sec", "0"}}));
int old_val = counter;
env_->SleepForMicroseconds(10000000);
ASSERT_EQ(counter, old_val);
}

static void assert_candidate_files_empty(DBImpl* dbfull, const bool empty) {
dbfull->TEST_LockMutex();
JobContext job_context(0);
Expand Down