8000 [Merged by Bors] - Topic level compression fix by morenol · Pull Request #2264 · infinyon/fluvio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Merged by Bors] - Topic level compression fix #2264

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Topic level compression fix #2264

wants to merge 5 commits into from

Conversation

morenol
Copy link
Contributor
@morenol morenol commented Mar 24, 2022 •
edited
Loading

Closes #2265

Adding #[fluvio(min_version) in Replica struct was provoking using the default for compression type.

This changes fixes that

@morenol morenol force-pushed the lmm/spu-compression-topic-level branch 2 times, most recently from d9d4a57 to 537dbce Compare March 24, 2022 16:36
@morenol morenol requested a review from sehz March 24, 2022 16:37
sehz
sehz requested changes Mar 24, 2022
View reviewed changes
Copy link
Contributor
@sehz sehz left a comment

Choose a reason for hiding this comment

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

(1) Can you create issue for this?
(2) Add unit test or integration test to verify this is fixed.

@morenol
Copy link
Contributor Author
morenol commented Mar 24, 2022 •
edited
Loading

I had an integration test for this https://github.com/infinyon/fluvio/blob/master/crates/fluvio-spu/src/services/public/tests/produce.rs, seems to be working as expected but the issue is that there, the ReplicaLocalStore is mocked.

Not sure how to test this in another way since I cannot create producer with invalid compression algorithm

@sehz
Copy link
Contributor
sehz commented Mar 24, 2022

#2265 should have steps to reproduce. That should be put into integration test.

@morenol
Copy link
Contributor Author
morenol commented Mar 24, 2022

I added in the description how to replicate that, but still it requires the usage of old client version, not sure how to add a test with that constraint

@sehz
Copy link
Contributor
sehz commented Mar 24, 2022

I think that scenario (old client) to new cluster is already described in current CI tests @tjtelan?

@morenol
Copy link
Contributor Author
morenol commented Mar 24, 2022

I think that scenario (old client) to new cluster is already described in current CI tests @tjtelan?

Yes, but that is something that we will only be able to test when old client is 0.9.21

@morenol
Copy link
Contributor Author
morenol commented Mar 24, 2022 •
edited
Loading

I can add a fixed CLI integration test using fluvio 0.9.21, but that looks weird to me

@morenol morenol force-pushed the lmm/spu-compression-topic-level branch from 537dbce to ab47207 Compare March 24, 2022 17:11
@morenol morenol marked this pull request as draft March 24, 2022 17:36
@morenol morenol force-pushed the lmm/spu-compression-topic-level branch 3 times, most recently from d640838 to 64d42e7 Compare March 25, 2022 14:18
@morenol morenol marked this pull request as ready for review March 25, 2022 14:20
@morenol morenol marked this pull request as draft March 25, 2022 14:20
@morenol morenol force-pushed the lmm/spu-compression-topic-level branch 3 times, most recently from 9007245 to 8f5d99f Compare March 25, 2022 15:56
@morenol morenol force-pushed the lmm/spu-compression-topic-level branch from 8f5d99f to 4a24c43 Compare March 25, 2022 16:32
@morenol morenol marked this pull request as ready for review March 25, 2022 19:10
@morenol morenol requested a review from sehz March 25, 2022 19:10
@morenol morenol changed the title Lmm/spu compression topic level Topic level compression fix Mar 25, 2022
sehz
sehz approved these changes Mar 25, 2022
View reviewed changes
Copy link
Contributor
@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM

@sehz
Copy link
Contributor
sehz commented Mar 25, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 25, 2022
@morenol
Topic level compression fix (#2264)
e52dffd 
Closes #2265

Adding #[fluvio(min_version) in Replica struct was provoking using the default for compression type. 

This changes fixes that
@bors
Copy link
bors bot commented Mar 25, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Topic level compression fix [Merged by Bors] - Topic level compression fix Mar 25, 2022
@bors bors bot closed this Mar 25, 2022
@morenol morenol deleted the lmm/spu-compression-topic-level branch May 19, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sehz sehz sehz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SPU does not return error code if produce request is using a compression algorithm not accepted by topic configuration
2 participants
@morenol @sehz
0