Refactor the PusherConsumer interactions
#9133
Merged
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
gotjosh
commented
Aug 29, 2024
| @@ -24,21 +24,28 @@ import ( | |||
| "github.com/grafana/mimir/pkg/util/spanlogger" | |||
| ) | |||
|
|
|||
| const shardForSeriesBuffer = 2000 // TODO dimitarvdimitrov 2000 is arbitrary; the idea is that we don't block the goroutine calling PushToStorage while we're flushing. A linked list with a sync.Cond or something different would also work | |||
|
|
|||
| type Pusher interface { | |||
| PushToStorage(context.Context, *mimirpb.WriteRequest) error | |||
| } | |||
|
|
|||
| type PusherCloser interface { | |||
There was a problem hiding this comment.
I also wanted to refactor this but turned out to be too big of a change and decided against it.
gotjosh
commented
Aug 29, 2024
4 tasks
4 tasks
There was a problem hiding this comment.
nice one! I think there's a place where we can have a deadlock and a place where checking the channel every time is too expensive. Otherwise, this is definitely an improvement.
You mentioned some race condition, but I couldn't spot any. Isn't the test failure caused by different series sharding on different CPU architectures?
4 tasks
gotjosh
force-pushed
the
gotjosh/ingester/refactor-test
branch
from
a55e417
to
08a724a
Compare
- Introduce a new `BatchingQueue` to reveal the intentions of a queue per shard. - Removed the need to call `Close` as we pushed data into TSDB from the main loop. This was really confusing given we were using close as a semantic to "no more items are coming" and ensure any incomplete batches were done. - Renamed and moved the `noopPusherCloser` which is more of an alternative way to push data without any sort of concurrency. - Inlined a few methods to get rid of certain level of indirection that made the code harder to understand.
gotjosh
force-pushed
the
gotjosh/ingester/refactor-test
branch
from
08a724a
to
8a8a7c2
Compare
gotjosh
merged commit 16f8a12
into
dimitar/ingester/consume-latency-push-sharding
27 of 29 checks passed
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.
What this PR does
BatchingQueueto reveal the intentions of a queue per shard.Closeas we pushed data into TSDB from the main loop. This was really confusing given we were using close as a semantic to "no more items are coming" and ensure any incomplete batches were done.noopPusherCloserwhich is more of an alternative way to push data without any sort of concurrency.Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.