Make SystemClock into a Customizable Class #8636

mrambacher · 2021-08-09T19:48:38Z

Made SystemClock into a Customizable class, complete with CreateFromString.

Cleaned up some of the existing SystemClock implementations that were redundant (NoSleep was the same as the internal one for MockEnv).

Changed MockEnv construction to allow Clock to be passed to the Memory/MockFileSystem.

zhichao-cao · 2021-08-20T19:17:20Z

include/rocksdb/system_clock.h

  // The name of this system clock
  virtual const char* Name() const = 0;

+  static const char* kDefaultName() { return "DefaultClock"; }


Why we need the kDefaultName() here?

There are two classes (Posix and Windows) which are both default, so both of these classes return true for IsInstanceOf(kDefaultName).

Additionally, you can get an instance of the default system clock via SystemClock::CreateFromString(kDefaultName, ...)

And address PR comments

During merge, the registration of the MockSystemClock was missed

facebook-github-bot · 2021-08-23T11:57:07Z

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

zhichao-cao · 2021-08-23T22:10:37Z

db/blob/blob_file_builder_test.cc

@@ -108,7 +109,7 @@ class BlobFileBuilderTest : public testing::Test {
    ASSERT_EQ(footer.expiration_range, ExpirationRange());
  }

-  MockEnv mock_env_;


Any essential need to change to std::unique_ptr?

It allows me to make the MockEnv constructor private. The MockFileSystem requires a Clock at the time it is being created, which requires two steps.

zhichao-cao · 2021-08-23T22:29:16Z

env/mock_env.h

+  explicit FakeSleepSystemClock(const std::shared_ptr<SystemClock>& c)
+      : SystemClockWrapper(c), fake_sleep_micros_(0) {}
+
+  void SleepForMicroseconds(int micros) override {


I may use "FakeSleepForMicroseconds" here and also rename the "MockSleepForMicroseconds" to "FakeSleepForMicroseconds", what do you think. Otherwise, the name itself does not provide the information that we "Fake" the sleep action.

The class is FakeSleepSystemClock. It is meant to override a "Sleep" with a fake. I am not sure how the renaming does what you want?

Right, but after user has something like std::unique_ptr<FakeSleepSystemClock> clock, the will just will clock->SleepForMicroseconds, which can be misleading in the code. I may prefer we keep the "Fake" in the function name.

This is meant to be a test class that simulates a sleep by just keeping track of the delay and not actually sleeping. If the internal code calls SleepForMicroseconds and this method is called "FakeSleepForMicroseconds", this method/class will have no effect.

The point of this class is to NOT do a real Sleep when the code (internal or test) calls Sleep.

The SpecialEnv class does something similar to sometimes (but not always) override the true sleep.

zhichao-cao

LGTM, thanks!

facebook-github-bot · 2021-08-25T00:41:02Z

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

facebook-github-bot · 2021-08-30T14:07:05Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-08-30T14:08:42Z

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

facebook-github-bot · 2021-08-30T14:13:51Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-08-30T14:14:15Z

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

This is the implementation of a SystemClock present in SpecialEnv. The clock is used for MockEnv if no clock is specified. For the DBTests, the mock env is wrapped by another SpecialEnv with this clock, so there is no reason to do the wrapping again. At some point, the SpecialEnv will be refactored into a SpecialFS/CompositeEnvWrapper and can then take advantage of this same class.

facebook-github-bot · 2021-09-14T18:30:31Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-14T18:32:16Z

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

facebook-github-bot · 2021-09-14T18:33:19Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-14T18:33:44Z

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

zhichao-cao · 2021-09-17T19:47:34Z

env/mock_env.cc


-  const char* Name() const override { return "MockSystemClock"; }
+MockEnv::MockEnv(Env* env, const std::shared_ptr<FileSystem>& fs,


The new TimeElapesSystemClock class looks good to me in general (the name may not be that appropriate......). Can you describe what's the testing failure when using MockSystemClock and why it is resolved in current implementation?

The TimeElapseSystemClock was extracted from SpecialEnv. Feel free to suggest a better name. SpecialClock sounded nondescript and there is another class already called MockSystemClock.

There were some failures with the "FakeSystemClock" when running under MEM_ENV. The problem is that the SpecialEnv would do a "Sleep" and defer to the wrapper clock, which in this case was a FakeSystemClock that did a fake sleep. Tests that were depending on a real sleep would therefore fail. I believe the failures were typically around the rate limiter tests but my recollection may be off.

zhichao-cao · 2021-09-17T21:37:25Z

env/time_elapse_clock.h

+
+namespace ROCKSDB_NAMESPACE {
+// A SystemClock that can "mock" sleep and counts its operations.
+class TimeElapseSystemClock : public SystemClockWrapper {


how about "TimeEmulatedSystemClock"?

zhichao-cao · 2021-09-17T21:40:06Z

env/time_elapse_clock.h

+
+  bool IsTimeElapseOnlySleep() const { return time_elapse_only_sleep_.load(); }
+  void SetMockSleep(bool enabled = true) { no_slowdown_ = enabled; }
+  bool UseMockSleep() const { return no_slowdown_; }


"UseMockSleep()" -> "IsMockSleepUsed()"

Rename TimeElapsedSystemClock to EmulatedSystemClock, to address PR comments.

facebook-github-bot · 2021-09-18T00:18:05Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

zhichao-cao

LGTM, do remember to mention in HISTORY.md that customizable class is extended to SystemClock. And also, even EmulatedSystemClock is not a public API, we may need to inform user this new class.

facebook-github-bot · 2021-09-21T11:52:16Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-21T13:20:27Z

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

facebook-github-bot · 2021-09-21T16:23:56Z

@mrambacher merged this pull request in 6924869.

Summary: Made SystemClock into a Customizable class, complete with CreateFromString. Cleaned up some of the existing SystemClock implementations that were redundant (NoSleep was the same as the internal one for MockEnv). Changed MockEnv construction to allow Clock to be passed to the Memory/MockFileSystem. Pull Request resolved: facebook/rocksdb#8636 Reviewed By: zhichao-cao Differential Revision: D30483360 Pulled By: mrambacher fbshipit-source-id: cd0e3a876c39f8c98fe13374c06e8edbd5b9f2a1

mrambacher added 2 commits August 9, 2021 10:22

Make SystemClock into a Customizable Object

5a480cf

Add more tests

2ec9455

mrambacher requested a review from zhichao-cao August 9, 2021 19:48

facebook-github-bot added the CLA Signed label Aug 9, 2021

mrambacher added 3 commits August 9, 2021 17:03

Fix LITE build

adf0a06

Fix LITE mode

9719ebe

Fix test failures

ef8d5bd

zhichao-cao reviewed Aug 20, 2021

View reviewed changes

mrambacher added 2 commits August 22, 2021 13:19

Merge to main

b879a7f

And address PR comments

Fix merge issue

2ac6202

During merge, the registration of the MockSystemClock was missed

zhichao-cao reviewed Aug 23, 2021

View reviewed changes

zhichao-cao approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into CustomSystemClock

f499cfb

Fix merge issue

d7e0c7d

mrambacher added 2 commits September 14, 2021 10:13

Merge to latest

f6aab6a

Another make format...

9414fb5

zhichao-cao reviewed Sep 17, 2021

View reviewed changes

Address PR Comments

fd7bfdc

Rename TimeElapsedSystemClock to EmulatedSystemClock, to address PR comments.

zhichao-cao approved these changes Sep 20, 2021

View reviewed changes

mrambacher added 2 commits September 21, 2021 07:49

Merge branch 'main' into CustomSystemClock

0c9a0b0

Update HISTORY for SystemClock

ff85b83

facebook-github-bot closed this in 6924869 Sep 21, 2021

facebook-github-bot added the Merged label Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SystemClock into a Customizable Class #8636

Make SystemClock into a Customizable Class #8636

mrambacher commented Aug 9, 2021

zhichao-cao Aug 20, 2021

mrambacher Aug 22, 2021

facebook-github-bot commented Aug 23, 2021

zhichao-cao Aug 23, 2021

mrambacher Aug 23, 2021

zhichao-cao Aug 23, 2021

mrambacher Aug 23, 2021

zhichao-cao Aug 24, 2021 •

edited

Loading

mrambacher Aug 24, 2021

zhichao-cao left a comment

facebook-github-bot commented Aug 25, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

zhichao-cao Sep 17, 2021

mrambacher Sep 17, 2021

zhichao-cao Sep 17, 2021

zhichao-cao Sep 17, 2021

facebook-github-bot commented Sep 18, 2021

zhichao-cao left a comment

facebook-github-bot commented Sep 21, 2021

facebook-github-bot commented Sep 21, 2021

facebook-github-bot commented Sep 21, 2021


		const char* Name() const override { return "MockSystemClock"; }
		MockEnv::MockEnv(Env* env, const std::shared_ptr<FileSystem>& fs,

Make SystemClock into a Customizable Class #8636

Make SystemClock into a Customizable Class #8636

Conversation

mrambacher commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 23, 2021

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

zhichao-cao Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhichao-cao left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 25, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Aug 30, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

facebook-github-bot commented Sep 14, 2021

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 Sep 18, 2021

zhichao-cao left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 21, 2021

facebook-github-bot commented Sep 21, 2021

facebook-github-bot commented Sep 21, 2021

zhichao-cao Aug 24, 2021 •

edited

Loading