Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Retrying fanout delivery can duplicate entries already accepted by an earlier endpoint.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents loki.write from panicking after failed updates by blocking entries until a consumer becomes available.
Changes:
- Adds consumer readiness synchronization and stopped-consumer retries.
- Tests failed-update recovery for WAL and non-WAL modes.
- Refactors existing benchmarks.
File summaries
| File | Description |
|---|---|
internal/component/loki/write/write.go |
Adds consumer lifecycle synchronization and retry logic. |
internal/component/loki/write/write_test.go |
Adds recovery tests and reorganizes benchmarks. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _ = consumer.ConsumeEntry(ctx, e) | ||
| // Only a stopped consumer is worth waiting on. Anything else is an accepted | ||
| // entry, a canceled context while shutting down, or a failed WAL write. | ||
| if err := consumer.ConsumeEntry(ctx, e); !errors.Is(err, loki.ErrConsumerStopped) { |
Contributor
Author
There was a problem hiding this comment.
I would say this is acceptable, it is better to risk that a entry get sent twice than always dropping it.
This file contains hidden or 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
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.
Brief description of Pull Request
No longer panic on a failed update.
Pull Request Details
Before if we could not successfully create a client during an update the next entry this component got would trigger a panic since
c.consumerwould be nil.The naive fix would be to do a nil check in
consumeEntryand just return if it was nil but this would silently drop entries. So instead we takeconsumerandconsumerReadyunder a read lock. If consumer is nil we wait on this channel. This will stop the pipeline until either a successful update have been made or component is stopped. I also added a retry forloki.ErrConsumerStoppedsince we will only get this error if a client consumer is stopped while we try to enqueue entry and it's safe for us to retry when we have a new consumer.Issue(s) fixed by this Pull Request
Notes to the Reviewer
PR Checklist