-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[chore][exporter] Consolidate merge splitting for the case where maxLimit is set and the case it's not #12104
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12104 +/- ##
==========================================
+ Coverage 91.71% 91.84% +0.12%
==========================================
Files 463 465 +2
Lines 24803 25296 +493
==========================================
+ Hits 22749 23232 +483
- Misses 1672 1675 +3
- Partials 382 389 +7 ☔ View full report in Codecov by Sentry. |
3cbe092
to
c9eee19
Compare
qb.currentBatchMu.Unlock() | ||
continue | ||
} | ||
if mergeSplitErr != nil || reqList == nil { |
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.
Not sure you want to do anything if error, unless I am missing something.
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.
merge error is reserved for things like failed type check, and means the request should not be retried. We want to remove the request (by calling onProcessingFinished
) from the queue in that case.
qb.currentBatchMu.Unlock() | ||
continue | ||
} | ||
if mergeSplitErr != nil || reqList == nil { |
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.
A slice never compare to nil, you must most likely compare len to 0
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.
Hmmm. In Go a slice can be nil and the comparison is valid. Note however that nil != []type{}
.
https://go.dev/tour/moretypes/12
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.
Makes sense! I updated reqList == nil
check to len(reqList) == 0
.
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.
Hmmm. In Go a slice can be nil and the comparison is valid. Note however that nil != []type{}.
https://go.dev/tour/moretypes/12
I know that :) It is just that in practice we should always compare len with 0.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if len is 0, get a segfault here?
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.
According to the exporter.request
specification, the result is guaranteed to be non-empty if there is no error.
// marked as not mutable. The length of the returned slice MUST not be 0. |
But to be safe, I added a check and return early if length is 0
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. |
54a57f3
to
6077b1a
Compare
Closing this PR as the two paths are consolidated in #12260. |
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