docs(server): say what the evict-on-full check actually tests - #1209
Open
astrogilda wants to merge 1 commit into
Open
docs(server): say what the evict-on-full check actually tests#1209astrogilda wants to merge 1 commit into
astrogilda wants to merge 1 commit into
Conversation
The eviction predicate is queue fullness at delivery time. The docstrings and the warning described it as an abandoned subscriber whose consumer stopped draining, which is a claim about the consumer that the check never makes: a consumer that is reading steadily but has fallen max_queue_size events behind satisfies the same predicate and is evicted the same way. Name the queue bound instead, and say explicitly that eviction does not imply the consumer stopped reading, so an operator reading the warning does not go hunting for a dead consumer and find a live one. No behavior change. Signed-off-by: Sankalp Gilda <sankalp.gilda@gmail.com>
🧪 Code Coverage (vs
|
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.
Follow-up to #1137, documentation only. No behavior change: the diff touches two docstrings, one comment and one log format string. Ruff's linter and formatter are both clean on the two files, and the 73 tests in the event-queue and active-task modules pass.
#1137 added eviction so one sink whose queue has filled cannot wedge the dispatcher for every other subscriber. The behavior is right and the words around it are not. The guard is
if sink._evict_on_full and sink.queue.full():, which tests queue fullness at delivery time and says nothing at all about the consumer. The docstring and the warning both described it as an abandoned subscriber whose consumer had stopped draining, which is a claim the check never makes. A consumer that is alive and reading steadily, merelymax_queue_sizeevents behind the dispatcher, satisfies the same predicate and is evicted the same way.That gap costs an operator real time. The warning as written sends someone looking for a dead consumer, and what they will find is a live one that is slow, with nothing in the message saying that is expected. So the warning now says the queue was full at delivery time and that the sink is therefore at least max_queue_size events behind, and it states outright that this does not mean the consumer stopped reading.
Alongside it, the delivery docstring gains one paragraph saying what the test is and what it is not, the evict_on_full parameter doc names the queue bound rather than abandonment, and the comment at the subscriber tap site says the same thing. Abandonment stays where it is genuinely the subject: the teardown docstring still describes a producer wedged closing an abandoned sink, and the regression test still calls its never-drained sink abandoned, because in both of those cases that is what is happening.