-
Notifications
You must be signed in to change notification settings - Fork 472
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
Fix config set compression type didn't take effect #1576
Conversation
201c9bd
to
9fe9911
Compare
9fe9911
to
f82a9c7
Compare
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.
the code does not look well to me, some fixes need to be pushed
2957f68
to
46eb2ea
Compare
I think the |
@PragmaTwice @xiaobiaozhao @caipengbo I minor refactor the PR, PTAL again. |
I have tested if the config set command works on my side: Set the compression to LZ4:
then use
|
Co-authored-by: Twice <twice@apache.org>
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
Co-authored-by: git-hulk <hulk.website@gmail.com> Co-authored-by: Twice <twice@apache.org>
Co-authored-by: git-hulk <hulk.website@gmail.com> Co-authored-by: Twice <twice@apache.org>
In this PR, we also refactor the c-style ConfigEnum array into the std::vector, the main reason is to make it easier to adopt dynamic config enums like the compression types. And for the test case, it's a bit hard to test if the compression type takes effect in underlying storage because we only enabled it when the levels >= 2. But will check it manually to make sure if works.
This closes #1560