-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: adding explicit batching #107
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 85.35% 86.18% +0.82%
==========================================
Files 14 15 +1
Lines 1127 1194 +67
==========================================
+ Hits 962 1029 +67
Misses 137 137
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@vlastahajek, thanks for your PR 👍. I have a small request regarding the formatting of our CHANGELOG.md
:
Co-authored-by: Jakub Bednář <jakub.bednar@gmail.com>
influxdb3/batching/batcher.go
Outdated
} | ||
|
||
// Add a metric to the batcher and call the given callbacks if any | ||
func (b *Batcher) Add(p *influxdb3.Point) { |
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.
I was wondering if this could be a variadic function, so that []*influxdb3.Point
could be used as an argument as well, or whether we might want o support adding []*influxdb3.Point
in another function.
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.
Good point. Variadic parameters look good.
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.
Looks Good. But I have two questions.
- Shouldn't there be a function in
batcher
to add a slice or array of*influxdb3.Point
instead of just a single point. - I noticed that using Option methods
WithSize()
andWithCapacity()
it is possible to create a batcher with Size > Capacity. My understanding of Golang is thatappend
will be lenient and reallocate as needed, but then the value ofBatcher.capacity
will no longer represent the true capacity of the internal buffer. Are we OK with this?
|
5309171
to
bf9fb71
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.
Tests pass locally. Examples work. Looks Good.
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.
Awesome! Thanks @vlastahajek!
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 🚀
Closes #103
Proposed Changes
Added
Batcher
for simple explicit batchingChecklist