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

Jump to bottom

Set max_background_flushes dynamically #6701

Closed
wants to merge 1 commit into from
Closed

Set max_background_flushes dynamically #6701

wants to merge 1 commit into from

Conversation

akankshamahajan15
Copy link
Contributor

Summary: 1. Add changes so that max_background_flushes can be set dynamically.
2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the
max_background_flushes dynamically using SetDBOptions.

TestPlan: 1. make -j64 check
2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads

facebook-github-bot
facebook-github-bot reviewed Apr 14, 2020
View reviewed changes
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.

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

@anand1976 anand1976 requested a review from ajkr April 14, 2020 23:39
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. Re-import the pull request

facebook-github-bot
facebook-github-bot reviewed Apr 15, 2020
View reviewed changes
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.

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

ajkr
ajkr approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM, a couple small comments.

db/db_options_test.cc Outdated
Reopen(options);
ASSERT_EQ(1, dbfull()->TEST_BGFlushesAllowed());
ASSERT_OK(dbfull()->SetDBOptions({{"max_background_flushes", "3"}}));
auto stop_token = dbfull()->TEST_write_controler().GetStopToken();
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this GetStopToken() come from the compaction equivalent of this test? I think it is relevant there but not here. It's needed there because RocksDB has a feature to limit compaction concurrency to one until compaction falls behind, after which it increases the limit to max_background_compactions. That feature intends to help in multi-process scenarios (like MyRocks) to allocate more compaction bandwidth to the RocksDB instances that really need it.

The way it detects compaction fell behind is via these WriteController tokens. So obtaining the token here would cause the number of compactions allowed to increase from one to max_background_compactions. But AFAIK there is no analogous feature for flush, so we shouldn't need to acquire a token here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we don't need this. It is required in compaction test case. I will remove this one. Thanks!

db/db_options_test.cc
ASSERT_OK(dbfull()->SetDBOptions({{"max_background_flushes", "3"}}));
auto stop_token = dbfull()->TEST_write_controler().GetStopToken();
ASSERT_EQ(3, dbfull()->TEST_BGFlushesAllowed());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we check env_->GetBackgroundThreads(Env::Priority::HIGH) increase from one to three as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure. I will add that check. Thanks!

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. Re-import the pull request

facebook-github-bot
facebook-github-bot reviewed Apr 20, 2020
View reviewed changes
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.

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

facebook-github-bot
facebook-github-bot reviewed Apr 20, 2020
View reviewed changes
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.

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

@akankshamahajan15
Set max_background_flushes dynamically
256895a 
Summary: 1. Add changes so that max_background_flushes can be set dynamically.
         2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the max_background_flushes dynamically
            using SetDBOptions.

TestPlan: 1. make -j64 check
          2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads
@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 has updated the pull request. Re-import the pull request

facebook-github-bot
facebook-github-bot reviewed Apr 20, 2020
View reviewed changes
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.

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

@facebook-github-bot
Copy link
Contributor

@akankshamahajan15 merged this pull request in 03a1d95.

@tabokie tabokie mentioned this pull request Oct 22, 2020
Merged
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request Oct 22, 2020
@akankshamahajan15 @tabokie
Set max_background_flushes dynamically (facebook#6701)
c766bb3 
Summary:
1. Add changes so that max_background_flushes can be set dynamically.
                   2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the
                        max_background_flushes dynamically using SetDBOptions.

TestPlan:  1. make -j64 check
                  2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads
Pull Request resolved: facebook#6701

Reviewed By: ajkr

Differential Revision: D21028010

Pulled By: akankshamahajan15

fbshipit-source-id: 5f949e4a8fd3c32537b637947b7ee09a69cfc7c1

Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit to tikv/rocksdb that referenced this pull request Oct 22, 2020
@tabokie @akankshamahajan15
Set max_background_flushes dynamically (facebook#6701) (#201)
7e6c07d 
Summary:
1. Add changes so that max_background_flushes can be set dynamically.
2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the
    max_background_flushes dynamically using SetDBOptions.

TestPlan:  1. make -j64 check
                2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads
Pull Request resolved: facebook#6701

Reviewed By: ajkr

Differential Revision: D21028010

Pulled By: akankshamahajan15

fbshipit-source-id: 5f949e4a8fd3c32537b637947b7ee09a69cfc7c1

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Akanksha Mahajan <akankshamahajan@fb.com>
tabokie added a commit to tabokie/rocksdb that referenced this pull request Nov 2, 2020
@tabokie @akankshamahajan15
Set max_background_flushes dynamically (facebook#6701) (tikv#201)
3e73921 
Summary:
1. Add changes so that max_background_flushes can be set dynamically.
2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the
    max_background_flushes dynamically using SetDBOptions.

TestPlan:  1. make -j64 check
                2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads
Pull Request resolved: facebook#6701

Reviewed By: ajkr

Differential Revision: D21028010

Pulled By: akankshamahajan15

fbshipit-source-id: 5f949e4a8fd3c32537b637947b7ee09a69cfc7c1

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Akanksha Mahajan <akankshamahajan@fb.com>
@tabokie tabokie mentioned this pull request Nov 2, 2020
Merged
yiwu-arbug pushed a commit to tikv/rocksdb that referenced this pull request Nov 2, 2020
@tabokie @akankshamahajan15
Set max_background_flushes dynamically (facebook#6701) (#201) (#204)
c7b0e57 
Summary:
1. Add changes so that max_background_flushes can be set dynamically.
2. Add a testcase DBOptionsTest.SetBackgroundFlushThreads which set the
    max_background_flushes dynamically using SetDBOptions.

TestPlan:  1. make -j64 check
                2. Using new testcase DBOptionsTest.SetBackgroundFlushThreads
Pull Request resolved: facebook#6701

Reviewed By: ajkr

Differential Revision: D21028010

Pulled By: akankshamahajan15

fbshipit-source-id: 5f949e4a8fd3c32537b637947b7ee09a69cfc7c1

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Akanksha Mahajan <akankshamahajan@fb.com>

Co-authored-by: Akanksha Mahajan <akankshamahajan@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ajkr ajkr ajkr approved these changes

@anand1976 anand1976 Awaiting requested review from anand1976

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akankshamahajan15 @facebook-github-bot @ajkr