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

Add lz4 compression #786

Merged
merged 5 commits into from
Nov 22, 2016
Merged

Add lz4 compression #786

merged 5 commits into from
Nov 22, 2016

Conversation

rtreffer
Copy link
Contributor

This patch may cause issues with kafka < 0.10.0 as it doesn't handle "broken" framing.
It should thus only be used with 0.10+.

@rtreffer rtreffer mentioned this pull request Nov 22, 2016
Copy link
Contributor

@eapache eapache left a comment

Choose a reason for hiding this comment

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

Code looks fine. Two requests:

  • Please add a test in message_test.go?
  • Please guard this against the configured Kafka version so that if somebody tries to use it against an older Kafka version they get a useful error instead of weird misbehaviour.

@rtreffer
Copy link
Contributor Author

config.Validate checks for versions < V0_10 and errs if LZ4 is enabled. Will add a message test later today.

@rtreffer
Copy link
Contributor Author

@eapache encode / decode test, config check + tests.

@rtreffer
Copy link
Contributor Author

Hm, the failed test looks like a false positive to me.

@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

Ya, I've rerun it. Sometimes the broker just doesn't start properly on CI for some reason.

@eapache
Copy link
Contributor

eapache commented Nov 22, 2016

Thanks!

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.

3 participants