Introduce a global StatsDumpScheduler for stats dumping #7223

jay-zhuang · 2020-08-06T16:00:12Z

Have a global StatsDumpScheduler for all DB instance stats dumping, including DumpStats() and PersistStats(). Before this, there're 2 dedicate threads for every DB instance, one for DumpStats() one for PersistStats(), which could create lots of threads if there're hundreds DB instances.

siying

It's awesome to see this progress!
Just two random thoughts:

we are assuming all DBs' status dumping is done by one thread. It might work but in case one dumping thread is stalling (e.g. DB is stalling), then all DBs are blocked. Even this is much better than what we have today, so I have no problem with going with this. It is worth considering that these threads are executed in thread pools instead. But it won't block this diff.
we would need timer for periodic compaction and TTL based compaction. It's worth considering what the code should look like after we have multiple of the tasks. Of course, we don't have to do over engineering in this first timer use case. Just some random thoughts.

monitoring/stats_dump_scheduler.cc

riversand963

Thanks @jay-zhuang for working on this task. Left a few comments.

TARGETS

db/db_impl/db_impl.cc

riversand963 · 2020-08-07T03:58:57Z

util/timer.h

@@ -119,11 +138,13 @@ class Timer {
        continue;
      }

-      FunctionInfo* current_fn = heap_.top();
+      auto current_fn = heap_.top();


I think it is better to use auto if the type can be easily told, which is not the case here. Maybe this is a matter of taste.

On second thought, I feel it's better to explicitly specify the type by saying FunctionInfo* current_fn.

sure, I'm wondering why?

We follow https://google.github.io/styleguide/cppguide.html#Type_deduction. To sum up, use type deduction only when it is clearer or safer, which is not the case here.

monitoring/stats_dump_scheduler.h

monitoring/stats_dump_scheduler.cc

util/timer.h

monitoring/stats_dump_scheduler.cc

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963

Thanks @jay-zhuang for the PR. Took a pass and left some comments.

db/db_impl/db_impl_open.cc

monitoring/stats_dump_scheduler.cc

monitoring/stats_dump_scheduler.h

monitoring/stats_history_test.cc

util/timer.h

facebook-github-bot · 2020-08-12T00:48:44Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

util/timer.h

util/timer_test.cc

facebook-github-bot · 2020-08-12T03:24:54Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-08-12T04:23:58Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

riversand963

Left two more comments, more to come. Can you also rebase? @jay-zhuang

db/db_impl/db_impl.cc

facebook-github-bot · 2020-08-13T01:40:27Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-08-13T04:45:18Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-08-13T05:18:10Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-08-13T16:39:01Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-08-13T19:18:35Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963

Overall LGTM with a few more comments. We are close to getting this done.

db/db_impl/db_impl.h

riversand963 · 2020-08-13T21:25:26Z

monitoring/stats_dump_scheduler.cc

+#ifndef NDEBUG
+bool StatsDumpScheduler::TEST_SetEnv(Env* env) {
+  MutexLock l(&test_mutex_);
+  auto scheduler = Default();


It feels a little weird to have a non-static setter method in which we get a pointer to a singleton and set the value for the singleton object.

I refactored it a little bit to create a StatsDumpTestScheduler, which takes Env for initializing new scheduler object, and contains all the test functions. It will be override with the Callback you mentioned below.

monitoring/stats_dump_scheduler.h

riversand963 · 2020-08-13T21:30:49Z

monitoring/stats_dump_scheduler_test.cc

+  Close();
+}
+
+TEST_F(StatsDumpSchedulerTest, MultiInstancesTest) {


Nit: s/MultiInstancesTest/MultiInstances/ to save some characters for the test name. This can apply to other tests.

monitoring/stats_dump_scheduler_test.cc

riversand963 · 2020-08-13T21:44:02Z

monitoring/stats_dump_scheduler_test.cc

+
+  std::string dbname = test::PerThreadDBPath("multi_env_test");
+  DB* db;
+  DB::Open(options2, dbname, &db);


Here we should check for the return value of Open by doing ASSERT_OK.

util/timer.h

riversand963 · 2020-08-13T21:47:49Z

util/timer.h

@@ -119,11 +138,13 @@ class Timer {
        continue;
      }

-      FunctionInfo* current_fn = heap_.top();
+      auto current_fn = heap_.top();


On second thought, I feel it's better to explicitly specify the type by saying FunctionInfo* current_fn.

util/timer.h

riversand963 · 2020-08-14T00:09:47Z

db/db_impl/db_impl.cc

-    }
+    stats_dump_scheduler_ = StatsDumpScheduler::Default();
+#ifndef NDEBUG
+    stats_dump_scheduler_->TEST_SetEnv(env_);


My main concern is about the difference in behavior for production and test code. Env::Default() is used in production, while env_ is used in tests. I am thinking about the following.
Add a sync point in this function.

TEST_SYNC_POINT_CALLBACK("DBImpl::StartStatsDumpScheduler:SetEnv", stats_dump_scheduler_);

In your unit test,

SyncPoint::GetInstance()->SetCallBack("DBImpl::StartStatsDumpScheduler:SetEnv", [&](void* arg) { auto* stats_dump_scheduler = reinterpret_cast<StatsDumpScheduler*>(arg); ASSERT_NE(nullptr, stats_dump_scheduler); stats_dump_scheduler->TEST_SetEnv(env_); });

This way, most of the unit tests will have the same behavior as production.

Yeah, that would be better.

facebook-github-bot · 2020-08-14T16:46:23Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-08-14T17:09:21Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-08-14T18:26:12Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963

LGTM with a few minor comments. Thanks @jay-zhuang for the PR.

riversand963 · 2020-08-15T00:04:51Z

util/timer.h

@@ -42,23 +42,30 @@ class Timer {
        running_(false),
        executing_task_(false) {}

-  ~Timer() {}


Curious to know the reason for the removal of destructor?

It's an empty non-virtual destructor, I think it would be better to be removed. Or should we?

riversand963 · 2020-08-15T00:05:40Z

util/timer.h

+      cond_var_.SignalAll();
+    } else {
+      // If it already exists, overriding it.
+      it->second->fn = std::move(fn_info->fn);
+      it->second->valid = true;
+      it->second->next_run_time_us = env_->NowMicros() + start_after_us;
+      it->second->repeat_every_us = repeat_every_us;
+      cond_var_.SignalAll();


We need only one cond_var_.SignalAll() in the end, right?

Have a global StatsDumpScheduler for all DB instance stats dumping, including DumpStats() and PersistStats(). Before this, there're 2 dedicate threads for every DB instance, one for DumpStats() one for PersistStats(), which could create lots of threads if there're hundreds DB instances.

facebook-github-bot · 2020-08-15T00:43:45Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-08-15T04:39:10Z

@jay-zhuang merged this pull request in 69760b4.

Summary: Have a global StatsDumpScheduler for all DB instance stats dumping, including `DumpStats()` and `PersistStats()`. Before this, there're 2 dedicate threads for every DB instance, one for DumpStats() one for PersistStats(), which could create lots of threads if there're hundreds DB instances. Pull Request resolved: facebook#7223 Reviewed By: riversand963 Differential Revision: D23056737 Pulled By: jay-zhuang fbshipit-source-id: 0faa2311142a73433ebb3317361db7cbf43faeba

facebook-github-bot added the CLA Signed label Aug 6, 2020

jay-zhuang force-pushed the thread branch from b7153a6 to 9796d0d Compare August 6, 2020 17:13

siying reviewed Aug 6, 2020

View reviewed changes

monitoring/stats_dump_scheduler.cc Show resolved Hide resolved

riversand963 reviewed Aug 7, 2020

View reviewed changes

jay-zhuang changed the title ~~[WIP] Add stats dump scheduler~~ Introduce a global StatsDumpScheduler for stats dumping Aug 11, 2020

jay-zhuang requested a review from riversand963 August 11, 2020 17:41

facebook-github-bot reviewed Aug 11, 2020

View reviewed changes

riversand963 reviewed Aug 11, 2020

View reviewed changes

riversand963 reviewed Aug 12, 2020

View reviewed changes

util/timer.h Show resolved Hide resolved

riversand963 reviewed Aug 12, 2020

View reviewed changes

util/timer_test.cc Outdated Show resolved Hide resolved

riversand963 reviewed Aug 12, 2020

View reviewed changes

db/db_impl/db_impl.cc Show resolved Hide resolved

db/db_impl/db_impl.cc Outdated Show resolved Hide resolved

jay-zhuang force-pushed the thread branch from 965cf8c to 4f41699 Compare August 13, 2020 01:40

jay-zhuang force-pushed the thread branch from 30a928a to 152f9eb Compare August 13, 2020 05:17

jay-zhuang force-pushed the thread branch from 152f9eb to f60a9aa Compare August 13, 2020 16:18

jay-zhuang requested a review from riversand963 August 13, 2020 16:18

facebook-github-bot reviewed Aug 13, 2020

View reviewed changes

riversand963 reviewed Aug 14, 2020

View reviewed changes

jay-zhuang requested a review from riversand963 August 14, 2020 17:01

facebook-github-bot reviewed Aug 14, 2020

View reviewed changes

riversand963 approved these changes Aug 15, 2020

View reviewed changes

jay-zhuang added 7 commits August 14, 2020 17:41

Review feedbacks

d38d873

Fix flaky test

91686f6

Refactor the test

74143d8

Fix lite build

4c01045

Const

598cde7

Add history

fb14b82

jay-zhuang force-pushed the thread branch from fbb7ddf to fb14b82 Compare August 15, 2020 00:43

facebook-github-bot reviewed Aug 15, 2020

View reviewed changes

facebook-github-bot closed this in 69760b4 Aug 15, 2020

facebook-github-bot added the Merged label Aug 15, 2020

thatsafunnyname mentioned this pull request Sep 16, 2020

Update master to version 6.13 #7378

Closed

Introduce a global StatsDumpScheduler for stats dumping #7223

Introduce a global StatsDumpScheduler for stats dumping #7223

Conversation

jay-zhuang commented Aug 6, 2020 • edited Loading

siying left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 12, 2020

facebook-github-bot commented Aug 12, 2020

facebook-github-bot commented Aug 12, 2020

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 13, 2020

facebook-github-bot commented Aug 13, 2020

facebook-github-bot commented Aug 13, 2020

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 13, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 14, 2020

facebook-github-bot commented Aug 14, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 14, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 15, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 15, 2020

jay-zhuang commented Aug 6, 2020 •

edited

Loading