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.

Sign up for GitHub

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

Jump to bottom

enhancement(config): Refactor the batching configuration #1154

Merged
merged 6 commits into from Nov 8, 2019
Merged

enhancement(config): Refactor the batching configuration #1154

merged 6 commits into from Nov 8, 2019

Conversation

bruceg
Copy link
Member

@bruceg bruceg commented Nov 5, 2019 •
edited

This is the first step at issue #1090, with more to come later. This consolidates the batching configuration into a common structure.

@bruceg
Refactor all batch_{size,timeout} sink configs into one struct
605ec9f 
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
@bruceg
Merge all the batch unwraps
469898e 
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
@bruceg
Merge the batch parameters to batched_with_{max,min}
dee71ef 
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
binarylogic
binarylogic reviewed Nov 6, 2019
View reviewed changes
scripts/generate/templates/_partials/_component_sections.md.erb Outdated
Comment on lines 39 to 40
1. The batch age meets or exceeds the configured `batch_timeout` (default: `<%= component.options.batch_timeout.human_default %>`).
2. The batch size meets or exceeds the configured `batch_size` (default: `<%= component.options.batch_size.human_default %>`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting: I asked Bruce to remove this until I can fix an issue accessing sub-options through the metadata objects.

lukesteensen
lukesteensen reviewed Nov 6, 2019
View reviewed changes
src/sinks/aws_cloudwatch_logs/mod.rs Outdated Show resolved Hide resolved
src/sinks/util/batch.rs Outdated Show resolved Hide resolved
@bruceg
Remove the extra Option level on each BatchConfig
ecb843d 
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
@binarylogic
Copy link
Contributor

👍 on this. Just a reminder, I'd like to wait to merge this since it is a breaking change. We'll be treating https://github.com/timberio/vector/milestone/25 as it's own release.

@binarylogic binarylogic changed the title fix(config): Refactor the batching configuration improvement(config)!: Refactor the batching configuration Nov 6, 2019
@binarylogic binarylogic changed the title improvement(config)!: Refactor the batching configuration enhancement(config)!: Refactor the batching configuration Nov 6, 2019
@binarylogic binarylogic added the meta: breaking change Anything that breaks backward compatibility. label Nov 6, 2019
@bruceg
Copy link
Member Author

bruceg commented Nov 6, 2019

+1 on this. Just a reminder, I'd like to wait to merge this since it is a breaking change.

Since the configuration and settings are separate in this, and thanks to serde(flatten), I can easily rework this to avoid any actual breaking change.

@bruceg
Revert the breaking configuration changes
65b073f 
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
@bruceg bruceg removed the meta: breaking change Anything that breaks backward compatibility. label Nov 6, 2019
@bruceg bruceg changed the title enhancement(config)!: Refactor the batching configuration enhancement(config): Refactor the batching configuration Nov 6, 2019
@bruceg
Copy link
Member Author

bruceg commented Nov 7, 2019

This is ready for re-review.

lukesteensen
lukesteensen approved these changes Nov 8, 2019
View reviewed changes
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

Nice!

@bruceg
Copy link
Member Author

bruceg commented Nov 8, 2019

Thanks.

It's a small detail, but what do you think about renaming BatchConfig.unwrap_or to .unwrap_with since it's not unwrapping an Option or Result which is normally associated with unwrap_or? Does it matter?

@lukesteensen
Copy link
Member

Logically you can think of it as an unwrap_or mapped over each of the possible fields, so I think it's a reasonably good name.

@bruceg bruceg merged commit f4adfd7 into master Nov 8, 2019
@bruceg bruceg deleted the refactor-batch branch November 8, 2019 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binarylogic binarylogic binarylogic left review comments

@lukesteensen lukesteensen lukesteensen approved these changes

@LucioFranco LucioFranco Awaiting requested review from LucioFranco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Config consistency & cleanup
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bruceg @binarylogic @lukesteensen