-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Feature/ingester stream backpressure #6217
[RFC] Feature/ingester stream backpressure #6217
Conversation
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0.4%
+ distributor 0.3%
+ querier 0%
+ querier/queryrange 0%
- iter -0.4%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
fbe7ba5
to
519ba84
Compare
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0.4%
- distributor -0.3%
+ querier 0%
+ querier/queryrange 0%
- iter -0.4%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
471b791
to
220c443
Compare
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0.4%
+ distributor 0%
+ querier 0.1%
+ querier/queryrange 0%
- iter -0.8%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
1 similar comment
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0.4%
+ distributor 0%
+ querier 0.1%
+ querier/queryrange 0%
- iter -0.8%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0.4%
+ distributor 0%
+ querier 0.1%
+ querier/queryrange 0%
- iter -0.8%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few (not so important) things. Just for curiosity, what would the backpressure improve? and since you used the term RFC, do you have a doc or anything explaining the reasoning behind the idea?
if err != nil { | ||
return &logproto.AckResponse{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same error from line 573)
if err != nil { | |
return &logproto.AckResponse{}, err | |
} |
queryIngester := instance.queries[req.Id] | ||
instance.queryMtx.Unlock() | ||
if queryIngester != nil { | ||
queryIngester.ReleaseAck() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm nitpicking maybe you should call it SendAck()
instead of ReleaseAck()
? (since SendAck will send an empty struct message to the channel)
I dare say between #6241 and this that these are the two main causes of ingester memory peaks. Whether or not this is the direction loki team want to go is worthy of comment. |
Hi! This issue has been automatically marked as stale because it has not had any We use a stalebot among other tools to help manage the state of issues in this project. Stalebots are also emotionless and cruel and can close issues which are still very relevant. If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry. We regularly sort for closed issues which have a We may also:
We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task, |
Go away |
Hi! This issue has been automatically marked as stale because it has not had any We use a stalebot among other tools to help manage the state of issues in this project. Stalebots are also emotionless and cruel and can close issues which are still very relevant. If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry. We regularly sort for closed issues which have a We may also:
We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task, |
Go away |
What this PR does / why we need it:
This PR add backpressure to the response to ingester
Query()
. A similar aproach could also be used forQuerySample()
This is done through the addition of an
Ack(id)
method and a limit to the max in-flight responses in the stream (currently 20)This is built upon my branch for #6216
Which issue(s) this PR fixes:
I suspect would fix #5804
Special notes for your reviewer:
This needs a cleanup and represents a RFC on the concept which it will get if positively reviewed.
Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md