Job queue #247
Merged
Job queue #247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
replay
reviewed
Jun 6, 2022
replay
reviewed
Jun 6, 2022
replay
reviewed
Jun 6, 2022
replay
reviewed
Jun 6, 2022
|
@replay Thanks for your review. I've addressed your comments. I've also modified implementation to avoid using I would appreciate if you had a chance to look at the PR again. |
replay
reviewed
Jun 7, 2022
Added test that pop() returns all elements from closed queue.
replay
approved these changes
Jun 7, 2022
|
Thanks for your review @replay! |
pstibrany
added a commit
to grafana/mimir
that referenced
this pull request
Jun 7, 2022
Included changes: grafana/mimir-prometheus#131 grafana/mimir-prometheus#247 These should result is lower memory usage by chunk mapper.
1 task
pracucci
reviewed
Jun 8, 2022
Comment on lines
252
to
253
| // When the queue is full and blocked on the writer the chunkRefMap has one more job than the cap of the jobCh | ||
| // because one job is currently being processed and blocked in the writer. |
There was a problem hiding this comment.
This comment should be updated too.
There was a problem hiding this comment.
I think comment in queueSize() needs to be updated too.
There was a problem hiding this comment.
Oh, thanks for catching this. Will send new PR.
pstibrany
added a commit
to grafana/mimir
that referenced
this pull request
Jun 8, 2022
Included changes: grafana/mimir-prometheus#131 grafana/mimir-prometheus#247 These should result is lower memory usage by chunk mapper.
Merged
pstibrany
added a commit
to grafana/mimir
that referenced
this pull request
Jun 8, 2022
Included changes: grafana/mimir-prometheus#131 grafana/mimir-prometheus#247 These should result is lower memory usage by chunk mapper.
pstibrany
added a commit
to grafana/mimir
that referenced
this pull request
Jun 8, 2022
* Update Prometheus with async chunk mapper changes. Included changes: grafana/mimir-prometheus#131 grafana/mimir-prometheus#247 These result is lower memory usage by chunk mapper. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
|
Sent to upstream Prometheus as prometheus/prometheus#10874. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reimplements
chan chunkWriteJobwith custom buffered queue that should use less memory, because it doesn't preallocate entire buffer for maximum queue size at once. Instead it allocates individual "segments" with smaller size.As elements are added to the queue, they fill individual segments. When elements are removed from the queue (and segments), empty segments can be thrown away. This doesn't change memory usage of the queue when it's full, but should decrease its memory footprint when it's empty (queue will keep max 1 segment in such case).
(Going to test the change in our dev cluster)