[chore][exporter] Consolidate merge splitting for the case where maxLimit is set and the case it's not #12104
Conversation
sfc-gh-sili
requested review from
bogdandrutu,
dmitryax and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12104 +/- ##
==========================================
- Coverage 91.71% 91.69% -0.03%
==========================================
Files 463 463
Lines 24803 24774 -29
==========================================
- Hits 22749 22717 -32
- Misses 1672 1674 +2
- Partials 382 383 +1 ☔ View full report in Codecov by Sentry. |
sfc-gh-sili
force-pushed
the
sili-consolidate
branch
3 times, most recently
from
d08401e to
3cbe092
Compare
sfc-gh-sili
force-pushed
the
sili-consolidate
branch
from
3cbe092 to
c9eee19
Compare
| qb.currentBatchMu.Unlock() | ||
| continue | ||
| } | ||
| if mergeSplitErr != nil || reqList == nil { |
There was a problem hiding this comment.
Not sure you want to do anything if error, unless I am missing something.
| qb.currentBatchMu.Unlock() | ||
| continue | ||
| } | ||
| if mergeSplitErr != nil || reqList == nil { |
There was a problem hiding this comment.
A slice never compare to nil, you must most likely compare len to 0
There was a problem hiding this comment.
Hmmm. In Go a slice can be nil and the comparison is valid. Note however that nil != []type{}.
https://go.dev/tour/moretypes/12
| qb.currentBatch = &batch{ | ||
| req: reqList[0], | ||
| // If there was a split, we flush everything immediately. | ||
| if reqList[0].ItemsCount() >= qb.batchCfg.MinSizeItems || len(reqList) > 1 { |
There was a problem hiding this comment.
What if len is 0, get a segfault here?
|
I tried to understand this PR, but I found myself with background-level questions as in #12118, would love to see package-level documentation on the new relationship between the queue sender and batch sender, so that I can keep track of timeout and cancellation-related topics. Thank you @sfc-gh-sili for leading this effort. |
This PR consolidates merge splitting for the case where maxLimit is set and the case it's not. This should not have any effect from users' perspective.
Description
Link to tracking issue
Fixes #
Testing
Documentation