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

Fix context propagation for DisabledBatcher #12231

Merged

Conversation

bogdandrutu
Copy link
Member

Finishes the implementation started in #12225

@bogdandrutu bogdandrutu requested a review from a team as a code owner February 1, 2025 00:20
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.30%. Comparing base (4ee2b50) to head (f0c4433).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12231   +/-   ##
=======================================
  Coverage   91.30%   91.30%           
=======================================
  Files         464      464           
  Lines       25662    25662           
=======================================
  Hits        23430    23430           
  Misses       1819     1819           
  Partials      413      413           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bogdandrutu bogdandrutu force-pushed the disabled-batcher-context branch from 44c47e8 to cd7dae5 Compare February 1, 2025 00:52
@sfc-gh-sili
Copy link
Contributor

Thank you, Bogdan!
I agree that using the callback function is a cleaner approach compared to using an index. However, I'm not sure if we can handle partial error handling with a callback.

For example, consider this scenario: we start with a batch that contains request x, then we received another request y and this resulted in a split. Then, we have

  • Batch A that contains request x and part of request y
  • Batch B that contains the other part of request y

If batch A was sent out successfully but batch B failed, we still propagate the failure back to the queue?

@dmitryax
Copy link
Member

dmitryax commented Feb 1, 2025

@bogdandrutu please rebase

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu force-pushed the disabled-batcher-context branch from cd7dae5 to f0c4433 Compare February 1, 2025 02:29
@bogdandrutu
Copy link
Member Author

@dmitryax done

@bogdandrutu bogdandrutu enabled auto-merge February 1, 2025 02:48
@bogdandrutu bogdandrutu added this pull request to the merge queue Feb 1, 2025
Merged via the queue into open-telemetry:main with commit 7b671b8 Feb 1, 2025
53 checks passed
@bogdandrutu bogdandrutu deleted the disabled-batcher-context branch February 1, 2025 03:11
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