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

Add TableBuilderOptions::level and relevant changes #1335

Merged
merged 1 commit into from
Sep 18, 2016
Merged

Add TableBuilderOptions::level and relevant changes #1335

merged 1 commit into from
Sep 18, 2016

Conversation

rockeet
Copy link
Contributor

@rockeet rockeet commented Sep 13, 2016

For issue: #1333

I also modified other relevant code.

@ghost
Copy link

ghost commented Sep 13, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -54,15 +54,16 @@ struct TableBuilderOptions {
CompressionType _compression_type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind making TableBuilderOptions constructor explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'explicit' keyword for constructor is useful only when parameter number can be 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right ! please ignore my comment

@IslamAbdelRahman
Copy link
Contributor

LGTM, Thanks @rockeet, I have some minor comments.
Can you also add a simple unit test if possible ?

@siying
Copy link
Contributor

siying commented Sep 13, 2016

Passing level to table builder is fine to me. I think what is needed more is whether the file is the bottonmost level to me. I assume you only want to apply strong compression in the bottommost level, which you can get from Compaction::bottommost_level().

@rockeet
Copy link
Contributor Author

rockeet commented Sep 14, 2016

@IslamAbdelRahman Thanks for your review again. A unit test is not need now, because this code change does not impact any existing code, ---- there is no any existing code in rocksdb really using this newly added 'level'. The 'level' is only used in our custom TableFactory. Or you could give me a unit test scenario in rocksdb ?

@rockeet
Copy link
Contributor Author

rockeet commented Sep 14, 2016

@siying I used sub_compact->compaction->output_level() for TableBuilderOptions::level and didn't use Compaction::bottommost_level(), is this right? ref:https://github.com/rockeet/rocksdb/commit/88518a284e9996fd7e2e70d0fd4ccfe885190eae?diff=unified#diff-6e99ed9f03dfac4fb36ae1a1e5a770bbR1140

@IslamAbdelRahman
Copy link
Contributor

@rockeet, I believe what @siying meant was that the TableBuilder will now know the level number but what you really want to know is if this level is the bottommost level or not, and you can know what is the bottommost level by checking Compaction::bottommost_level()

@siying
Copy link
Contributor

siying commented Sep 14, 2016

@rockeet what you are doing is correct. I was just saying that, you could use a better indication of whether you need to use your compression, which is whether the file is for the bottom-most level.

@ghost ghost added the CLA Signed label Sep 16, 2016
@ghost
Copy link

ghost commented Sep 16, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@IslamAbdelRahman IslamAbdelRahman merged commit 4c3f449 into facebook:master Sep 18, 2016
@IslamAbdelRahman
Copy link
Contributor

Thanks @rockeet !

@liufei26 liufei26 mentioned this pull request Dec 11, 2021
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.

3 participants