-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
out_http: add gzip option #4528
out_http: add gzip option #4528
Conversation
216f211
to
3ca09fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I think we need to take into account the compression of buffer.
I commentted below on the details, so please check.
3ca09fe
to
c114ff4
Compare
c114ff4
to
5772d4d
Compare
I'd like to add test patterns like this: c684509 However, the case of sending the gzipped chunk as is fails in my environment. Failure: test_write_with_gzip[buffer_compress: "gzip", json_array: false](HTTPOutputTest::GZIP)
/home/daipom/work/fluentd/fluentd/test/plugin/test_out_http.rb:564:in `test_write_with_gzip'
561: result.content_type
562: )
563: assert_equal 'gzip', result.headers['content-encoding']
=> 564: assert_equal test_events, result.data
565: assert_not_empty result.headers
566: end
567: end
<[{"bool"=>true, "message"=>"hello", "num"=>10},
{"bool"=>false, "message"=>"hello", "num"=>11}]> expected but was
<[{"bool"=>true, "message"=>"hello", "num"=>10}]>
diff:
? [{"bool"=>true, "message"=>"hello", "num"=>10},
? ]
? ?
- {"bool"=>false, "message"=>"hello", "num"=>11}] |
I also noticed this in my testing, im assuming that the behavior is correct but not what the test is expecting. Its delivering a single gzipped message rather than batching a few messages in the request. I can add a few more tests to cover off on the different option combinations |
The issue here is that the test harness doesn't really handle getting multiple http requests for one test, and in the gzipped buffer case the messages will come one by one. Im not entirely sure how to fix this properly without doing something like this:
|
@rockliffelewis Thanks for considering it! The cause is that fluentd/lib/fluent/plugin/compressable.rb Lines 57 to 75 in c0cd1e6
So, as you say, it's generally not a problem. fluentd/lib/fluent/plugin/in_http.rb Lines 509 to 523 in c0cd1e6
So, if we consider a use case where forwarding the gzip data to What do you think about this? |
Ah i see! I didnt realise the buffer was doing something fancy like that. I wouldn't expect most HTTP receivers to implement that either so at best we would need a In my usecase I want to use |
Signed-off-by: Lewis Rockliffe <[email protected]>
5772d4d
to
6ef1411
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
I will wait a bit to merge for the other maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for your contribution!
Which issue(s) this PR fixes:
Fixes #4410
What this PR does / why we need it:
Adds a
compress gzip
option toout_http
to enable compressing the http request before sending it to save on bandwidthDocs Changes:
fluent/fluentd-docs-gitbook#514
Release Note: