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.

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

Fix/minimize mock_time_env.h dependencies #7426

Closed
wants to merge 3 commits into from

Conversation

pdillinger
Copy link
Contributor

Summary: (a) own copy of kMicrosInSecond
(b) out-of-line sync point code

Test Plan: FB internal

Summary: (a) own copy of kMicrosInSecond
(b) out-of-line sync point code

Test Plan: FB internal
Copy link
Contributor

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

LGTM, with the exception of LIB_SOURCES

CMakeLists.txt Outdated
@@ -731,6 +731,7 @@ set(SOURCES
table/table_factory.cc
table/table_properties.cc
table/two_level_iterator.cc
test_util/mock_time_env.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be in TEST_LIB_SOURCES and not LIB_SOURCES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just copying from sync_point.cc. I'll try it and see if I have difficulty working out the public as well as internal dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe TESTUTIL_SOURCE for cmake.

src.mk Outdated
@@ -178,6 +178,7 @@ LIB_SOURCES = \
table/table_factory.cc \
table/table_properties.cc \
table/two_level_iterator.cc \
test_util/mock_time_env.cc \
Copy link
Contributor

Choose a reason for hiding this comment

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

TEST_LIB_SOURCES?

@@ -62,29 +64,11 @@ class MockTimeEnv : public EnvWrapper {
// TODO: this is a workaround for the different behavior on different platform
// for timedwait timeout. Ideally timedwait API should be moved to env.
// details: PR #7101.
void InstallTimedWaitFixCallback() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the methods that use std::numeric_limits also be moved or should be included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

env.h -> limits is sufficient

Copy link
Contributor

@jay-zhuang jay-zhuang left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@pdillinger merged this pull request in ac1734d.

pdillinger added a commit that referenced this pull request Sep 23, 2020
Summary:
(a) own copy of kMicrosInSecond
(b) out-of-line sync point code

Pull Request resolved: #7426

Test Plan: FB internal

Reviewed By: ajkr

Differential Revision: D23861363

Pulled By: pdillinger

fbshipit-source-id: de6b1621dca2f7391c5ff72bad04a7613dc27527
codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
Summary:
(a) own copy of kMicrosInSecond
(b) out-of-line sync point code

Pull Request resolved: facebook#7426

Test Plan: FB internal

Reviewed By: ajkr

Differential Revision: D23861363

Pulled By: pdillinger

fbshipit-source-id: de6b1621dca2f7391c5ff72bad04a7613dc27527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants