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

[exporter] exporter queue Read() #11396

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

sfc-gh-sili
Copy link
Contributor

@sfc-gh-sili sfc-gh-sili commented Oct 8, 2024

Description

This PR adds a public function GetNextItem to queue (both persistent queue and bounded memory queue)

Why this change?
Instead of blocking until consumption of the item is done, we would like to separate the API for reading and committing consumption.

Before:
Consume(consumeFunc)

After:
idx, item = Read()
OnProcessingFinished(idx)

Link to tracking issue

#8122
#10368

Testing

Documentation

@sfc-gh-sili sfc-gh-sili requested a review from a team as a code owner October 8, 2024 11:48
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.14%. Comparing base (9701538) to head (145d901).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11396   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files         432      432           
  Lines       20297    20296    -1     
=======================================
  Hits        18702    18702           
+ Misses       1232     1231    -1     
  Partials      363      363           

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

@mx-psi mx-psi requested a review from dmitryax October 8, 2024 13:50
exporter/internal/queue/queue.go Outdated Show resolved Hide resolved
exporter/internal/queue/bounded_memory_queue.go Outdated Show resolved Hide resolved
@sfc-gh-sili sfc-gh-sili force-pushed the sili-queue-batcher-read branch 5 times, most recently from 5834f7a to 95adb3d Compare October 9, 2024 11:24
@sfc-gh-sili sfc-gh-sili changed the title [exporter] exporter queue GetNextItem() [exporter] exporter queue Read() Oct 9, 2024
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM on the new API

exporter/internal/queue/bounded_memory_queue.go Outdated Show resolved Hide resolved
@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 9, 2024
@sfc-gh-sili sfc-gh-sili force-pushed the sili-queue-batcher-read branch 2 times, most recently from 4d78360 to 3a42e45 Compare October 10, 2024 20:43
Comment on lines +47 to +48
consumeErr := qc.consumeFunc(ctx, req)
qc.queue.OnProcessingFinished(index, consumeErr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
consumeErr := qc.consumeFunc(ctx, req)
qc.queue.OnProcessingFinished(index, consumeErr)
qc.queue.OnProcessingFinished(index, qc.consumeFunc(ctx, req))

@bogdandrutu bogdandrutu merged commit 28d0d57 into open-telemetry:main Oct 11, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 11, 2024
djaglowski pushed a commit to djaglowski/opentelemetry-collector that referenced this pull request Nov 21, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR adds a public function `GetNextItem` to queue (both persistent
queue and bounded memory queue)

Why this change?
Instead of blocking until consumption of the item is done, we would like
to separate the API for reading and committing consumption.

Before:
`Consume(consumeFunc)`

After:
`idx, item = Read()`
`OnProcessingFinished(idx)`

<!-- Issue number if applicable -->
#### Link to tracking issue

open-telemetry#8122
open-telemetry#10368

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants