Skip to content

compression: add zstd compressor and decompressor#1

Closed
rainingmaster wants to merge 9 commits intomainfrom
zstd
Closed

compression: add zstd compressor and decompressor#1
rainingmaster wants to merge 9 commits intomainfrom
zstd

Conversation

@rainingmaster
Copy link
Owner

@rainingmaster rainingmaster commented Mar 14, 2022

Commit Message: compression: add zstd compressor and decompressor
Additional Description: Like envoyproxy#12998, add new zstd(alson name as Zstandard) compression extensions in addition to gzip, brotli.
Risk Level: Low, add context for createCompressorFactoryFromProtoTyped in exist code.
Testing: uni tests, manual tests with curl.
Docs Changes: updated docs for compression and decompression HTTP filters to refer the new available encoder/decoder.
Release Notes: TODO

The PR adds a new dependency on https://github.com/facebook/zstd. Here's the current criteria answers:

Criteria Answer
Cloud Native Computing Foundation (CNCF) approved license BSD
Dependencies must not substantially increase the binary size unless they are optional TODO
No duplication of existing dependencies no other dep provides zstd
Hosted on a git repository and the archive fetch must directly reference this repository. https://github.com/facebook/zstd
CVE history appears reasonable, no pathological CVE arcs. so far 3 CVEs related to brotli have been registered
Code review (ideally PRs) before merge. PRs are reviewed before merge
Security vulnerability process exists, with contact details and reporting/disclosure process. https://github.com/facebook/zstd/blob/dev/CONTRIBUTING.md#issues
> 1 contributor responsible for a non-trivial number of commits. 242 contributors
Tests run in CI. CI set up with Travis CI
High test coverage (also static/dynamic analysis, fuzzing). Fuzzers are run in CI
Envoy can obtain advanced notification of vulnerabilities or of security releases. zstd is registered in CPE
Do other significant projects have shared fate by using this dependency? The Linux kernel has included Zstandard since November 2017 (version 4.14) as a compression method for the btrfs and squashfs filesystems
Releases (with release notes). https://github.com/facebook/zstd/releases
Commits/releases in last 90 days. last commit 2 days ago

Copy link

@rojkov rojkov left a comment

Choose a reason for hiding this comment

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

This looks great overall. I've got one high level question.

Choose a reason for hiding this comment

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

A little confuse here, has_set_dictionary_ is set to true, then hasSetDictionary return false?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @liuqiming-intel. Thanks for your review. has_set_dictionary_ is a tag to record is dictionary has try to set in this context(should be by request). So if has_set_dictionary_ is false, we will return false, and set it as true. So next round it will be true.

@rainingmaster
Copy link
Owner Author

Hi @rojkov @zhxie @liuqiming-intel , thanks for your review. I have update the code and comment, could you help me review again? Thanks a lot!

Comment on lines 19 to 22
Copy link

Choose a reason for hiding this comment

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

I think these constants should be updated to constexpr absl::string_view.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @zhxie , it is true absl::string_view is better than std::string, but here is relate to CompressorLibraryFactoryBase, should we update it when we upgrade all compressor module?

Copy link

Choose a reason for hiding this comment

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

Since we have already used const string&, then it is Ok to not touch it.

Copy link

Choose a reason for hiding this comment

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

Should this setting be bounded with lte and gte?

Copy link
Owner Author

@rainingmaster rainingmaster Mar 30, 2022

Choose a reason for hiding this comment

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

I think zstd is a little bit different with other compression algorithm, even it could be a negative number. as discussion here:

Setting a level does not automatically set all other compression parameters to default. Setting this will however eventually dynamically impact the compression parameters which have not been manually set.

And as my new test case, both 0 and 10000 are work for this parameter, so in fact I have no idea how to limit it...

@rainingmaster
Copy link
Owner Author

Raise a new PR in official git repo: envoyproxy#20434

@rainingmaster rainingmaster force-pushed the zstd branch 15 times, most recently from 2f58b23 to d55e0eb Compare March 28, 2022 07:34
@rainingmaster rainingmaster force-pushed the zstd branch 3 times, most recently from d55e0eb to 89a4642 Compare March 30, 2022 02:20
@rainingmaster rainingmaster force-pushed the zstd branch 5 times, most recently from 6397906 to f57666b Compare April 1, 2022 10:06
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
Signed-off-by: rainingmaster <jinhua.tan@bytedance.com>
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.

4 participants