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

feat: support user config manifest compression #1579

Merged
merged 3 commits into from
May 16, 2023
Merged

feat: support user config manifest compression #1579

merged 3 commits into from
May 16, 2023

Conversation

Taylor-lagrange
Copy link
Contributor

@Taylor-lagrange Taylor-lagrange commented May 14, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Expose ManifestLogStorage compress configuration to user

Manifest log now supports compression but it is not enabled by default. I hope to expose the option to users whether to enable Manifest log compression.

expose a config use_compress to user whether to use Manifest log compression.

[storage.manifest]
checkpoint_on_startup = true
use_compress = true # false by default

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

Closes #1394
Follow-up of #1497

@codecov
Copy link

codecov bot commented May 14, 2023

Codecov Report

Merging #1579 (6b944ed) into develop (4fc173a) will decrease coverage by 0.74%.
The diff coverage is 97.22%.

❗ Current head 6b944ed differs from pull request most recent head 9cbab16. Consider uploading reports for the commit 9cbab16 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1579      +/-   ##
===========================================
- Coverage    85.69%   84.96%   -0.74%     
===========================================
  Files          529      537       +8     
  Lines        85000    85208     +208     
===========================================
- Hits         72840    72393     -447     
- Misses       12160    12815     +655     

src/cmd/src/datanode.rs Outdated Show resolved Hide resolved
src/mito/src/config.rs Outdated Show resolved Hide resolved
@killme2008
Copy link
Contributor

Maybe we need to add a new test suite for compress=true in sqlness tests. To ensure all is fine when enabling compression for region and table manifests.

https://github.com/GreptimeTeam/greptimedb/blob/develop/tests/conf/standalone-test.toml.template

It's not urgent. Can you create an issue to track it?

Thank you.

@Taylor-lagrange
Copy link
Contributor Author

Consider add file datanode-manifest-compress-test.toml.template and standalone-manifest-compress-test.toml.template under path /tests/conf, then make it run and pass. Is any other needs to be consider? I have checked the code of sqlness runner quickly, only few line change will do.

@killme2008
Copy link
Contributor

Consider add file datanode-manifest-compress-test.toml.template and standalone-manifest-compress-test.toml.template under path /tests/conf, then make it run and pass. Is any other needs to be consider? I have checked the code of sqlness runner quickly, only few line change will do.

I think standalone-manifest-compress-test.toml.template is enough to cover. Thanks.

@killme2008
Copy link
Contributor

@Taylor-lagrange Will you add the sqlness test suite or leave it to next PR?

@Taylor-lagrange
Copy link
Contributor Author

Yes, I checked the sqlness code twice. My expected implementation conflicts with sqlness original implementation concept. Sqlness implementation is based on case, but if we have several standalone/distributed test then we copy those cases several times, it is not elegant. We should do it based on conf. But these need more in-depth discussion

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

@killme2008 killme2008 merged commit fb1ac0c into GreptimeTeam:develop May 16, 2023
@Taylor-lagrange
Copy link
Contributor Author

Sqlness code not complicate, maybe we can consider fork and make our own sqlness, The interface provided by the original code is not very flexible

@killme2008
Copy link
Contributor

Sqlness code not complicate, maybe we can consider fork and make our own sqlness, The interface provided by the original code is not very flexible

The sqlness maintainer is @waynexia , we can discuss this here or in slack channels.

@Taylor-lagrange Taylor-lagrange deleted the feat-compress-config branch May 16, 2023 03:11
QuenKar pushed a commit to QuenKar/greptimedb that referenced this pull request May 19, 2023
* feat: support user config manifest compression

* chore: change style

* chore: enhance test
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* feat: support user config manifest compression

* chore: change style

* chore: enhance test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compress manifest and checkpoint
3 participants