Skip to content

Fix DefaultBatcher implementation to handle multiple producers. #12244

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.

Sign up for GitHub

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

Jump to bottom
Closed
bogdandrutu opened this issue Feb 2, 2025 · 3 comments
Closed

Fix DefaultBatcher implementation to handle multiple producers. #12244

bogdandrutu opened this issue Feb 2, 2025 · 3 comments
Labels
bug Something isn't working

Comments

@bogdandrutu
Copy link
Member

Right now, if multiple producers are setup to talk to the DefaultBatcher it gets into a deadlock. Steps to reproduce remove all places where this issue is linked (force to allow only 1 producer).
@bogdandrutu bogdandrutu added the bug Something isn't working label Feb 2, 2025
@bogdandrutu bogdandrutu mentioned this issue Feb 2, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 2, 2025
@bogdandrutu
Revert "[exporter][queuebatcher] set feature gate UsePullingBasedExpo…
466c497 
…rterQueueBatcher to beta" (#12246)

Reverts #12122

During the revamp of the pull batching work, couple bugs were found:
1.
#12244
2.
#12247
@sfc-gh-sili sfc-gh-sili mentioned this issue Apr 24, 2025
Closed
@sfc-gh-sili
Copy link
Contributor

@bogdandrutu Would you mind sharing some insight to this issue? I’ve tried reproducing the deadlock using what I believe to be a particularly adverse setup, but haven’t had any success:

#12920

  • timeout is disabled
  • numConsumer is set to 2, but at least 5 requests are needed to reach minThreashold.

If you have any thoughts on conditions that might reproduce it, I’d really appreciate it.

@sfc-gh-bdrutu
Copy link

Hey, the bug is fixed for memory corruption point. It is only useless at this point to have multiple goroutines since everything in the batcher happens under a lock.

@dmitryax
Copy link
Member

@bogdandrutu Nice! I'll submit a PR to remove the hardcoded worker number = 1.

Hey, the bug is fixed for memory corruption point. It is only useless at this point to have multiple goroutines since everything in the batcher happens under a lock.

Not everything, just the batching itself. Sending the data out is still distributed over the workers, right? The batching part I guess will be addressed with the sharding that you proposed in #12473

@dmitryax dmitryax mentioned this issue Apr 25, 2025
Merged
dmitryax added a commit to dmitryax/opentelemetry-collector that referenced this issue Apr 25, 2025
@dmitryax
Do not ignore the num_consumers setting when batching is enabled
f42b88e 
Remove hardcoding the number of queue workers to one if batching is enabled. The bug described in open-telemetry#12244 isn't relevant anymore.
dmitryax added a commit to dmitryax/opentelemetry-collector that referenced this issue Apr 25, 2025
@dmitryax
Do not ignore the num_consumers setting when batching is enabled
0d1d2bc 
Remove hardcoding the number of queue workers to one if batching is enabled. The bug described in open-telemetry#12244 isn't relevant anymore.
github-merge-queue bot pushed a commit that referenced this issue Apr 25, 2025
@dmitryax
[exporterhelper] Do not ignore the num_consumers setting when batch…
f26c6bb 
…ing is enabled (#12921)

Remove hardcoding the number of queue workers to one if batching is
enabled. The bug described in
#12244
isn't relevant anymore.
@sourcery-ai sourcery-ai bot mentioned this issue Apr 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bogdandrutu @dmitryax @sfc-gh-sili @sfc-gh-bdrutu