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.

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

Closed
wants to merge 2 commits into from

Conversation

sfc-gh-sili
Copy link
Contributor

@sfc-gh-sili sfc-gh-sili commented Jan 16, 2025

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

@sfc-gh-sili sfc-gh-sili requested review from bogdandrutu, dmitryax and a team as code owners January 16, 2025 01:59
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base (42b83a7) to head (54a57f3).
Report is 61 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@sfc-gh-sili sfc-gh-sili force-pushed the sili-consolidate branch 4 times, most recently from 3cbe092 to c9eee19 Compare January 20, 2025 23:37
qb.currentBatchMu.Unlock()
continue
}
if mergeSplitErr != nil || reqList == nil {
Copy link
Member

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.

Copy link
Contributor Author

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 {
Copy link
Member

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Member

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 {
Copy link
Member

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?

Copy link
Contributor Author

@sfc-gh-sili sfc-gh-sili Jan 29, 2025

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

@jmacd
Copy link
Contributor

jmacd commented Jan 23, 2025

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.

@sfc-gh-sili
Copy link
Contributor Author

Closing this PR as the two paths are consolidated in #12260.

@sfc-gh-sili sfc-gh-sili closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants