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 dynamically adjust the upper limit of the data that each layer configure option. #497

Merged
merged 30 commits into from
Mar 6, 2022

Conversation

WyattJia
Copy link
Contributor

@git-hulk
Copy link
Member

Cool, thanks for your contribution.

@git-hulk git-hulk added the feature type new feature label Feb 24, 2022
@git-hulk
Copy link
Member

git-hulk commented Feb 24, 2022

@WyattJia I have a look at the PR, overall was good to me. But we need to fix those lint issues first:

src/config.cc:152:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
src/config.cc:153:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
src/config.cc:427:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/config.cc:428:  Lines should be <= 120 characters long  [whitespace/line_length] [2]

@WyattJia
Copy link
Contributor Author

@WyattJia I have a look at the PR, overall was good to me. But we need to fix those lint issues first:

src/config.cc:152:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
src/config.cc:153:  Lines should be <= 120 characters long  [whitespace/line_length] [2]
src/config.cc:427:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
src/config.cc:428:  Lines should be <= 120 characters long  [whitespace/line_length] [2]

Copy that, I've formated and pushed code, thanks for your review.

kvrocks.conf Outdated Show resolved Hide resolved
src/config.h Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
caipengbo
caipengbo previously approved these changes Feb 25, 2022
kvrocks.conf Outdated Show resolved Hide resolved
kvrocks.conf Outdated Show resolved Hide resolved
kvrocks.conf Outdated Show resolved Hide resolved
WyattJia and others added 2 commits February 25, 2022 14:36
@WyattJia WyattJia changed the title Add rocksdb level_compaction_dynamic_level_bytes option configuration support. Add dynamically adjust the upper limit of the data that each layer configure option. Feb 25, 2022
caipengbo
caipengbo previously approved these changes Feb 25, 2022
Copy link
Contributor

@caipengbo caipengbo left a comment

Choose a reason for hiding this comment

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

Good job!

src/config.cc Outdated Show resolved Hide resolved
tests/config_test.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
git-hulk
git-hulk previously approved these changes Feb 26, 2022
.gitignore Outdated Show resolved Hide resolved
Co-authored-by: Wang Yuan <[email protected]>
src/config.cc Outdated Show resolved Hide resolved
kvrocks.conf Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

git-hulk commented Mar 6, 2022

cool, also need to multiple the MiB in config's callback like RocksDB.write_buffer_size

https://github.com/KvrocksLabs/kvrocks/blob/unstable/src/config.cc#L346

@WyattJia
Copy link
Contributor Author

WyattJia commented Mar 6, 2022

cool, also need to multiple the MiB in config's callback like RocksDB.write_buffer_size

https://github.com/KvrocksLabs/kvrocks/blob/unstable/src/config.cc#L346

Done and thank you for your reminder.

@git-hulk
Copy link
Member

git-hulk commented Mar 6, 2022

@WyattJia Cool, really thanks for your contribution and patient.

@git-hulk
Copy link
Member

git-hulk commented Mar 6, 2022

@ShooterIT Please take a look again

Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

LGTM

@git-hulk git-hulk merged commit 84cdbc7 into apache:unstable Mar 6, 2022
Comment on lines +436 to +438
if (!RocksDB.level_compaction_dynamic_level_bytes) {
return Status(Status::NotOK, errNotSetLevelCompactionDynamicLevelBytes);
}
Copy link
Member

Choose a reason for hiding this comment

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

this judgement is necessary? a mistake?

# The total file size of level-1 sst.
#
# Default: 256 M
rocksdb.max_bytes_for_level_base 256
Copy link
Member

Choose a reason for hiding this comment

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

since this config has bytes word, we should use byte unit, right? or we rename it to level_base_size? maybe rocksdb name is better?

This commit is not released in newest version, we have chances to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I also think bytes is better.

@ShooterIT
Copy link
Member

@WyattJia have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature type new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants