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 configure compression level #240

Merged
merged 14 commits into from
Nov 23, 2022

Conversation

tisonkun
Copy link
Contributor

This closes #239.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as draft November 15, 2022 17:47
@tisonkun
Copy link
Contributor Author

According to #239 (comment), pending to add a compression abstraction to hold these configs.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
This should be derived once 10XGenomics/lz4-rs#30 released.
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun marked this pull request as ready for review November 15, 2022 18:28
@tisonkun
Copy link
Contributor Author

Updated.

cc @zamazan4ik @FlorentinDUBOIS @DonghunLouisLee

Since I change the type of compression config key, it's possible that we need a major version bump.

Signed-off-by: tison <wander4096@gmail.com>
@@ -422,26 +423,26 @@ impl<Exe: Executor> TopicProducer<Exe> {

let topic = topic.clone();
let batch_size = options.batch_size;
let compression = options.compression;
let compression = options.compression.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be Copy. Blocked by upstream 10XGenomics/lz4-rs#30.

@zamazan4ik
Copy link

FMPOV looks fine. Thank you for the PR!

@tisonkun tisonkun changed the title feat: support configure zstd compression level feat: support configure compression level Nov 16, 2022
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

@zamazan4ik do you have a suggestion on which version to release including this patch? 4.3.0 or 5.0.0.

Semantically we should release 5.0.0 but actually this crate is not stable yet I would say (refer #224).

@zamazan4ik
Copy link

@zamazan4ik No, I have no. For me personally, there is no considerable difference :) So I suggest just bumping the version to 4.3.0 .

src/compression.rs Show resolved Hide resolved
src/compression.rs Show resolved Hide resolved
src/producer.rs Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

Updated. cc @fantapsody @FlorentinDUBOIS

@tisonkun tisonkun force-pushed the support-compression-level-zstd branch from ab3de4b to c889a08 Compare November 20, 2022 10:22
@@ -422,27 +423,7 @@ impl<Exe: Executor> TopicProducer<Exe> {

let topic = topic.clone();
let batch_size = options.batch_size;
let compression = options.compression;

match compression {
Copy link
Contributor Author

@tisonkun tisonkun Nov 20, 2022

Choose a reason for hiding this comment

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

Now the compression modes are structs under control. We exclude the corresponding enum members if features are unset. Thus, we're sure that there's no config-implementation mismatch.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Nov 21, 2022

Pending to merge by the end of this week.

I'm going to release a 5.0.0 for breaking changes. @fantapsody @FlorentinDUBOIS @DonghunLouisLee what do you think?

For the increasing version number, the only risk I can see is that they're all beyond 1.0.0 but as stated in #224, they're actually untested for compatibility with a specific official Pulsar version.

@tisonkun
Copy link
Contributor Author

tisonkun commented Nov 23, 2022

Pending to merge tomorrow and stage as 5.0.0.

Perhaps I'll add rustfmt/clippy tools to reformat code stably before calling the release.

@tisonkun tisonkun merged commit eb0416e into streamnative:master Nov 23, 2022
@tisonkun tisonkun deleted the support-compression-level-zstd branch November 23, 2022 13:10
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.

Add control over compression level
4 participants