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

Make bottommost_temperature dynamically changeable #9402

Closed
wants to merge 5 commits into from
Closed

Make bottommost_temperature dynamically changeable #9402

wants to merge 5 commits into from

Conversation

jay-zhuang
Copy link
Contributor

Summary: Make AdvancedColumnFamilyOptions.bottommost_temperature
dynamically changeable with SetOptions API.

Test Plan: added unittest

zhichao-cao
zhichao-cao reviewed Jan 20, 2022
View reviewed changes
Copy link
Contributor

@zhichao-cao zhichao-cao left a comment

Choose a reason for hiding this comment

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

One general question, when we assign the temperature to the bottom-most level, is it still under the DB mutex protection?

@jay-zhuang jay-zhuang marked this pull request as ready for review January 20, 2022 01:02
@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@jay-zhuang
Copy link
Contributor Author

One general question, when we assign the temperature to the bottom-most level, is it still under the DB mutex protection?

This fix is only for updating the option, assigning the temperature only happens in the next compaction:
jay-zhuang@7c44641#diff-d737a8ca0e8d739e33c030d56fdfdb5da3c54a2772750eef1c3a7f022e2a54d3R6732-R6747

@zhichao-cao
Copy link
Contributor

One general question, when we assign the temperature to the bottom-most level, is it still under the DB mutex protection?

This fix is only for updating the option, assigning the temperature only happens in the next compaction: jay-zhuang@7c44641#diff-d737a8ca0e8d739e33c030d56fdfdb5da3c54a2772750eef1c3a7f022e2a54d3R6732-R6747

I see, so even when we call "SetOption" during compaction, current compaction will not be influenced. Thanks!

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

siying
siying reviewed Jan 21, 2022
View reviewed changes
Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

Thank you for working on it. Can you add test scenarios in option_test and options_settable_test to cover this new option?

.circleci/config.yml Outdated Show resolved Hide resolved
@jay-zhuang
Make bottommost_temperature dynamically changeable
863ab36 
Summary: Make `AdvancedColumnFamilyOptions.bottommost_temperature`
dynamically changeable with `SetOptions` API.

Test Plan: added unittest
@jay-zhuang
Fix build
19028cb
@jay-zhuang
Fix status check build
4a8ea94
@jay-zhuang
Add tests
a01e4d2
@jay-zhuang
Remove override
c0ce119
@facebook-github-bot
Copy link
Contributor

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@jay-zhuang
Copy link
Contributor Author

Thank you for working on it. Can you add test scenarios in option_test and options_settable_test to cover this new option?

Sure, updated the tests to include that.

siying
siying approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@siying siying 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 improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhichao-cao zhichao-cao zhichao-cao left review comments

@siying siying siying approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@jay-zhuang @facebook-github-bot @zhichao-cao @siying