-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Improve SubCompaction Partitioning #10393
Conversation
This should be the implementation of #1842(Request: Optimization for subcompact: Add Get Random Keys) |
aca0e91
to
8a26d0c
Compare
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
c50a7bb
to
05bba31
Compare
@siying has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
@siying has updated the pull request. You must reimport the pull request before landing. |
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I think we should mention that it's a behavior change that sub-compaction number is more like be max out.
iiter_unique_ptr.reset(iiter); | ||
} | ||
|
||
const uint64_t kMaxNumAnchors = uint64_t{128}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to make it proportional to the size of the file, so small files don't have to sample that much, and make sure it also works okay for huge files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a comment. It's not easy to figure out inside the reader whether it is a smaller file or a larger file of the compaction.
@siying has updated the pull request. You must reimport the pull request before landing. |
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
828867a
to
c79d00b
Compare
@siying has updated the pull request. You must reimport the pull request before landing. |
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Unit tests still haven't been fixed. Also need to add more tests. But I ran some simple fillrandom db_bench and the partitioning feels reasonable. Pull Request resolved: facebook#10393 Test Plan: 1. Make sure existing tests pass. This should cover some basic sub compaction logic to be correct and the partitioning result is reasonable; 2. Add a new unit test to ApproximateKeyAnchors() 3. Run some db_bench with max_subcompaction = 4 and watch the compaction is indeed partitioned evenly. Reviewed By: jay-zhuang Differential Revision: D38043783 fbshipit-source-id: 085008e0f85f9b7c5abff7800307618320efb19f
Summary:
Unit tests still haven't been fixed. Also need to add more tests. But I ran some simple fillrandom db_bench and the partitioning feels reasonable.
Test Plan: