Conversation
… stable The concurrency test compared loki_process_cri_partial_lines_flushed_total against every entry it appended, but FlushAll is the shutdown drain and does not touch that counter. How many entries are still buffered when the workers finish is timing dependent, so the assertion failed with a different count on every run. Count only the entries that left through FlushIfExceeded and compare against that. Also bound the up-front stripe allocation. max_partial_lines is user supplied and has no upper limit, so a large value allocated a large map in every stripe at construction. The maps grow on demand, so only the pre-allocation is capped. Also fix a truncated doc comment on partialLinesStriped, a missing space in a FIXME, a stale reference to ProcessBatch and ProcessEntry, and a lint failure from a trailing blank line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thampiotr
force-pushed
the
thampiotr/cri-partial-lines-fixups
branch
from
September 11, 2026 13:11
921eae7 to
5e075af
Compare
…scarded When the buffered partial line is already at max_partial_line_size, the fast path incremented loki_process_cri_lines_truncated_total even for an empty CRI partial record, where nothing is discarded. Increment only when the new line is non empty. A stored line never exceeds the limit, because every return path either stays under it or slices to exactly it, so an empty new line is the only way to reach the counter with nothing to discard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flush drains every stripe, so it hands back partial lines for streams the current batch never touched and forwards them through this caller's next. That is what makes the reordering intra stream rather than only between batches, and it is not obvious from reading FlushIfExceeded on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hutdown process appends every entry in a batch before it checks max_partial_lines once, so a batch can finish some partial lines, leave others open and trip the limit in one call. Nothing covered that combination. Send batches that do all three, then assert every piece of content reaches the output exactly once and the total length is unchanged. The test says nothing about how many downstream calls happen or how lines are grouped, so it does not break when that changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
loki_process_cri_partial_lines_flushed_total tracks lines released because max_partial_lines was exceeded, so FlushAll at shutdown leaves it alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checking max_partial_lines once per batch is deliberate. The limit exists to stop partial lines accumulating over time, so it does not need to be exact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FlushAll released buffered partial lines without recording them, so lines that never got their full line before shutdown had no signal at all. Count them, the cost is one counter add per flush rather than per line. That also makes the concurrent test assertion simpler, since every entry now leaves through a path that counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stop flushes stage state, so a Consume racing it can buffer an entry that nothing then forwards. Require callers to stop producers first, matching the contract ShardingConsumer already documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lp text The counter now also records partial lines released at shutdown, so the help text and the component reference no longer attribute it only to max_partial_lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
💻 Deploy preview available (chore: Tweaks to PR 6913): |
…unter The shutdown flush runs only from PipelineConsumer, which has no non-test callers, so the component reference would describe behaviour a user cannot observe yet. Leave it to whichever change enables the new pipeline. The metric help text keeps the wording, since it sits next to the code that counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Proposed changes to the PR 6931