-
Notifications
You must be signed in to change notification settings - Fork 1.4k
compressable:Zstd comp support #4657
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
Conversation
2b8c9d1
to
4143998
Compare
6bf74ae
to
0b79317
Compare
Thanks! I will see this this weekend! |
@daipom did u get chance to look at this? |
Sorry, I haven't made time for this. 😢 |
Its fine i guess, |
@Athishpranav2003 I have confirmed the entire direction! Sorry I haven't made time to see the detailed implementation, such as It looks basically good as The core logic would be the following.
It is complicated, but the compression of Buffer and Chunk and the flush process of output plugins are closely related.
|
@daipom thanks for the review. Yah, this will kick a lot more changes in the overall events pipeline. |
@daipom can you please check the change in the Compression module as well. Once if we merge this we can proceed with the other development around this |
Sure! I will review the
To judge if we can merge this only with the support of the module and It would be necessary to update |
Sure @daipom If it's needed then I can split the PR into 2 where this will only focus on |
@daipom how to proceed in this PR? |
Sorry, please give me a few more days 😢 |
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.
@Athishpranav2003
Sorry for my late response 😢
I have commented on some points, but basically it looks good! Thanks!
About our future policies, I think it is better not to separate the PRs, as these changes need to be considered in combination with updating Forward-Protocol.
How about supporting the out_forward
by implementing Output/Buffer/Chunk logic in this PR?
My concern is whether we can support Buffer/Chunk zstd compression.
To support it, the compressed data must be able to be concatenated.
gzip allows it.
It appears that we can concat zstd files without any problems, but I wonder if this is an officially supported specification of zstd and zstd-ruby
gem.
$ echo "Hello world" > 1
$ echo "Hello Fluentd" > 2
$ zstd -f 1
$ zstd -f 2
$ cat 1.zst 2.zst > 3.zst
$ zstd -d 3.zst
$ cat 3
Hello world
Hello Fluentd
0b79317
to
dbbd2f6
Compare
@daipom i addressed your comments but couldnt test it locally due to some issue. |
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.
Did not this code work?
I guess there is a small careless I will check after 18th |
Hmm, these changes would be necessary, but looks like the error in your environment is caused by another reason.
|
OK! I'm sorry my response was so slow, and it took so long. |
@daipom the gem issue got resolved after update,prolly some mismatched versions, All tests are passing now |
Signed-off-by: Athish Pranav D <[email protected]>
Signed-off-by: Athish Pranav D <[email protected]>
dbbd2f6
to
7d837b6
Compare
@daipom some doubts here in chuck open method for decompress we dont have any type identification. fluentd/lib/fluent/plugin/buffer/chunk.rb Lines 201 to 226 in 78a7972
|
Yah, |
@Athishpranav2003 About For example, for if @buffer.compress == :text
@buffer.compress = @compress unless @compress == :text
else
if @compress == :text
log.info "buffer is compressed. If you also want to save the bandwidth of a network, Add `compress` configuration in <match>"
elsif @compress != @buffer.compress
raise Fluent::ConfigError, "You cannot specify different compression formats for Buffer (Buffer: #{@buffer.compress}, Self: #{@compress})"
end
end As for implementation, there appears to be no other problems. If you have any other concerns, please let me know. We need to update the protocol before we merge this.
|
9696e0d
to
c44175c
Compare
@daipom i have addressed ur comments. Feel free to point out the nits as well. |
c44175c
to
dc13f85
Compare
@daipom |
Yes!! |
I have made an issue for this. Let's summarize the new protocol there, for now. |
@Athishpranav2003 |
Signed-off-by: Athish Pranav D <[email protected]>
Signed-off-by: Athish Pranav D <[email protected]>
dc13f85
to
cdb963f
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.
LGTM! Thanks for this significant improvement!!
This should be merged into the next minor version v1.19.
Also, we need to wait #4758.
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 so much!
In #4758, we now have a clear direction for protocol changes, so I merge this PR.
**Which issue(s) this PR fixes**: * Related to #4758 **What this PR does / why we need it**: When using `compress zstd` for `out_forward`, output the following warning log. ``` zstd compression feature is an experimental new feature supported since v1.19.0. Please make sure that the destination server also supports this feature before using it. in_forward plugin for Fluentd supports it since v1.19.0. ``` In #4758, it was agreed that we would refrain from updating [the forward protocol v1.5](https://github.com/fluent/fluentd/wiki/Forward-Protocol-Specification-v1.5) for the time being and instead output a log message indicating that this feature is experimental. > for now, we will make zstd available as an exprimenatal feature on the Fluentd side, and note in the documentation that it is an experimental feature that cannot be used unless the server supports it. > Just displaying as a warning log to tell: "This is experimental feature and not standardized yet" or similar message should be enough. * #4758 (comment) * #4758 (comment) * #4758 (comment) **Docs Changes**: Not needed. (It should be done as #4657) **Release Note**: Not needed. (A note for #4657 would be enough.) Signed-off-by: Daijiro Fukuda <[email protected]>
**Which issue(s) this PR fixes**: Fixes #5005 **What this PR does / why we need it**: This PR will fix RuntimeError with large zstd compressed data **Docs Changes**: Not needed because this feature has not been released yet. **Release Note**: Should be described with #4657 Signed-off-by: Shizuo Fujita <[email protected]>
Which issue(s) this PR fixes:
Fixes #4162
What this PR does / why we need it:
Adds new compression method support to handle messages.
zstd
tocompress
option.zstd
format. (Experimental)Docs Changes:
Release Note:
zstd
tocompress
option.zstd
tocompress
option.zstd
tocompress
option.zstd
format. (Experimental)