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

Set text compress as default option for FluentD file buffer #850

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

sumo-drosiek
Copy link
Contributor

Description

Set text compress as default due to fluent/fluentd#3110

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@sumo-drosiek sumo-drosiek requested review from a team, vsinghal13, perk-sumo and pmalek-sumo and removed request for a team August 24, 2020 17:37
@sumo-drosiek sumo-drosiek force-pushed the drosiek-disable-fluentd-buffer-compression branch 2 times, most recently from ecb9b9f to 9f28027 Compare August 24, 2020 17:39
Copy link
Contributor

@vsinghal13 vsinghal13 left a comment

Choose a reason for hiding this comment

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

Did we get an estimate of increase in the load due to text format?

Copy link
Contributor

@frankreno frankreno left a comment

Choose a reason for hiding this comment

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

one comment, otherwise LGTM

@@ -211,6 +211,7 @@ fluentd:
totalLimitSize: "128m"
retryMaxInterval: "10m"
retryForever: true
compress: text
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment that GZIP is not supported so it is clear why we set it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankreno I would say not recommended and point to the issue. Is it ok?

@sumo-drosiek
Copy link
Contributor Author

@vsinghal13 it was 10 times for my test data. It will be compression rate of the gzip for the given log files

@sumo-drosiek sumo-drosiek force-pushed the drosiek-disable-fluentd-buffer-compression branch from 82eaeb2 to 5d6510c Compare August 24, 2020 17:46
@sumo-drosiek sumo-drosiek merged commit 2949a65 into master Aug 24, 2020
@sumo-drosiek sumo-drosiek deleted the drosiek-disable-fluentd-buffer-compression branch August 24, 2020 18:05
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@perk-sumo perk-sumo added this to the v1.2 milestone Aug 28, 2020
@perk-sumo perk-sumo changed the title Set text compress as default Set text compress as default option for FluentD file buffer Aug 28, 2020
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.

4 participants