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 compression to elasticsearch output #853

Conversation

cookieshake
Copy link
Contributor

@cookieshake cookieshake commented Aug 26, 2021

Hello. I need a client-side compression for elasticsearch output, so I implemented this.

I did make test, make fmt, make lint.

I am using this commit in my service, and it seems it works well.

@cookieshake
Copy link
Contributor Author

I saw that the test failed. I read the contribution guideline again, and did make docs. Now I am running make test-integration but it fails. So I am investigating the cause.

@mihaitodor
Copy link
Collaborator

Hi @cookieshake and thank you for the contribution! I don't see anything in there that could make the integration tests fail, so it could just be a flakey test, but I'll let @Jeffail confirm.

@Jeffail
Copy link
Collaborator

Jeffail commented Aug 27, 2021

Hey @cookieshake, looks good but I think it's worth naming the field gzip_compression to be more explicit. Is the integration test specifically failing for elasticsearch? Because like @mihaitodor said there's some flakiness in there 😅

@Jeffail
Copy link
Collaborator

Jeffail commented Aug 28, 2021

Thanks @cookieshake, looks good!

@Jeffail Jeffail merged commit 5c14ca1 into redpanda-data:master Aug 28, 2021
@cookieshake
Copy link
Contributor Author

cookieshake commented Aug 30, 2021

@mihaitodor thank you for checking my code!

@Jeffail I changed compression to gzip_compression. The integration test failed because the dockerd of my pc was not working properly. I did the test on another ubuntu VM, and the test for output/writer was successful. But other tests like mssql test failed. Thank you for checking & merging!

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.

None yet

3 participants