loki: Improve Tailer loop #877
Conversation
|
@pracucci |
|
Thanks @sandlis for your quick feedback. I will work on testing the |
pracucci
force-pushed
the
improve-tailer-loop
branch
from
15d254c
to
7c196d3
Compare
|
@sandlis I've introduced tests and solved the TODOs I initially left in the code. The PR is now ready for a full review. May you take a look and share your thoughts, please? |
pkg/querier/tail.go
Outdated
| @@ -14,12 +14,22 @@ import ( | |||
|
|
|||
| const ( | |||
| // if we are not seeing any response from ingester, how long do we want to wait by going into sleep | |||
| nextEntryWait = time.Second / 2 | |||
| tailerWaitEntryThrottle = time.Second / 2 | |||
There was a problem hiding this comment.
I think we should move this to pkg/querier/querier.go since it is used in that file.
There was a problem hiding this comment.
Definitely. I was in doubt as well. You confirmed my doubt.
pkg/querier/tail.go
Outdated
| select { | ||
| case t.responseChan <- tailResponse: | ||
| droppedEntries = make([]droppedEntry, 0) |
There was a problem hiding this comment.
Lets put a check here for the size of droppedEntries before re-initializing?
If we are doing this, we would also want to put same condition above where droppedEntries is assigned to tailResponse i.e https://github.com/grafana/loki/pull/877/files#diff-6786afcc7f6bd0a7f048371a46467af0R152
There was a problem hiding this comment.
The way I read your feedback is "do it only if len(droppedEntries) > 0". If I correctly understood your comment, then definitely yes (done). If I misunderstood your comment, may you share more details on your thought, please?
There was a problem hiding this comment.
That is what I meant, thanks for fixing this!
pracucci
force-pushed
the
improve-tailer-loop
branch
from
562829f
to
f30e5ab
Compare
|
@sandlis Thanks for your review! I should have addressed both. |
What this PR does / why we need it:
The current
Tailer.loop()implementation is a bit hard to follow and presents some issues which I'm trying to address in this PR:isBlocked()was always returningfalsebecauset.blockedwas never set totrue: now switched toisResponseChanBlocked()droppedEntrieswas not populated with all entries in case of a droppedtailResponsecontaining multiple streams/entries (was just populated with the current one)droppedEntriescould grow indefinitely: now introduced an hard capmaxDroppedEntriesPerTailResponseTailer.loop()to make it easier to follow reading the code (and hopefully easier to maintain over the future)Notes to reviewers:
This is a draft PR cause I would like to hear if you believe the proposed changes make sense. If so, I will work on adding tests (we have no tests on the tailer right now, it's time to show it some ❤️ ).
Checklist