Skip to content
This repository has been archived by the owner on Jul 18, 2023. It is now read-only.

Add maxBatchSize parameter to IntervalBatcher #59

Merged
merged 1 commit into from
May 9, 2018

Conversation

cypressious
Copy link
Contributor

Fixes #58

Copy link
Contributor

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the PR. Couple of review items.

internal static class EnumerableExtensions
{
// from https://github.com/morelinq/MoreLINQ/blob/master/MoreLinq/Batch.cs
public static IEnumerable<IEnumerable<TSource>> Batch<TSource>(this IEnumerable<TSource> source, int size)
Copy link
Contributor

Choose a reason for hiding this comment

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

To comply with the license of the original code, the original copyright info has to be included here. (Apache License 2.0 section 4.c.).

@@ -4,6 +4,6 @@ namespace InfluxDB.Collector.Configuration
{
public abstract class CollectorBatchConfiguration
{
public abstract CollectorConfiguration AtInterval(TimeSpan interval);
public abstract CollectorConfiguration AtInterval(TimeSpan interval, int? maxBatchSize = null);
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 default this to a value in line with what InfluxDB suggests? (1000 to 5000?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an overload of this method that accepts the maxBatchSize instead, so that we can avoid bumping the major version? (Binary-incompatible change)

@cypressious
Copy link
Contributor Author

@nblumhardt Thx for the review. I addressed your comments.

@cypressious
Copy link
Contributor Author

I've squashed the commits

@nblumhardt
Copy link
Contributor

👍 thanks!

@nblumhardt nblumhardt merged commit bec2ce3 into influxdata:dev May 9, 2018
@cypressious cypressious deleted the feature-batch-count branch May 9, 2018 09:01
@cypressious
Copy link
Contributor Author

any chance we could get a (dev) release with this feature in it?

@nblumhardt
Copy link
Contributor

Thanks for the heads-up, @cypressious - looks like the build needs some attention. I'll raise an issue 👍

@vladchilom vladchilom mentioned this pull request Oct 16, 2019
@bednar bednar mentioned this pull request Apr 14, 2020
@bednar bednar added this to the 1.1.1 milestone Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batching can lead to data loss because of too large request sizes
3 participants