-
Notifications
You must be signed in to change notification settings - Fork 550
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
distributor: check for inflight bytes before reading httpgrpc message without message size in metadata. #6534
Conversation
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
3d7c135
to
f7ab718
Compare
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.
Thanks Peter for reviving this PR! I left a question.
pkg/distributor/distributor.go
Outdated
} | ||
|
||
func (d *Distributor) checkInflightBytes(inflightBytes int64) error { | ||
func (d *Distributor) checkInflightBytes(inflightBytes int64, rejectEqualInflightBytes bool) error { |
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.
The rejectEqualInflightBytes
looks a complication with a minimal benefit. I don't think we need a limit which is precise at the byte, but just good enough. Can we just remove it?
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.
Fair enough, I've simplified it in distributor.
Ingester has similar check, and I quickly tried to remove it too, but then tests got more complicated, so I left it as-is.
@@ -1109,17 +1118,6 @@ func (d *Distributor) startPushRequest(ctx context.Context, httpgrpcRequestSize | |||
return ctx, rs, nil | |||
} | |||
|
|||
func (d *Distributor) checkHttpgrpcRequestSize(rs *requestState, httpgrpcRequestSize int64) error { | |||
// If httpgrpcRequestSize was already checked, don't check it again. | |||
if rs.httpgrpcRequestSize > 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.
This check has been lost. Why it's not required anymore?
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.
It was moved outside of this function, into https://github.com/grafana/mimir/blob/bf5dd777ad18fdf6de0a5ddc76e11b34180ff2af/pkg/distributor/distributor.go#L1099
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.
I'm sorry. I'm still lost. The check here was on rs.httpgrpcRequestSize
to avoid double adding the size of 1 request to the accumulater. The check done in the line you linked is not on rs.httpgrpcRequestSize
.
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.
I see what you mean now. This check is in fact unnecessary. We only pass valid httpgrpcRequestSize
from public StartPushRequest
, which is called exactly once from gRPC handler. So we can't possibly try to check different httpgrpc sizes for the same request.
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.
I see what you mean and you're right. I think the check if rs.httpgrpcRequestSize > 0
was introduced to make the code safer against possible future changes. Given it's an extra if
condition doesn't look a bad idea to keep it to me (given how tricky is the wiring here) but I also don't feel strong about it.
…known. Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
f08cf5f
to
df67c58
Compare
@@ -1109,17 +1118,6 @@ func (d *Distributor) startPushRequest(ctx context.Context, httpgrpcRequestSize | |||
return ctx, rs, nil | |||
} | |||
|
|||
func (d *Distributor) checkHttpgrpcRequestSize(rs *requestState, httpgrpcRequestSize int64) error { | |||
// If httpgrpcRequestSize was already checked, don't check it again. | |||
if rs.httpgrpcRequestSize > 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.
I see what you mean and you're right. I think the check if rs.httpgrpcRequestSize > 0
was introduced to make the code safer against possible future changes. Given it's an extra if
condition doesn't look a bad idea to keep it to me (given how tricky is the wiring here) but I also don't feel strong about it.
Thank you for your contribution. This pull request has been marked as stale because it has had no activity in the last 150 days. It will be closed in 30 days if there is no further activity. If you need more time, you can add a comment to the PR. |
This pull request has been closed because it has been stale for 30 days with no activity. Feel free to reopen if you want to continue working on this. |
What this PR does
This is enhancement of #6300, this PR rejects push requests coming from httpgrpc if Distributor is already at inflight-bytes limit. Previously this check would be skipped if gRPC message did not pass request size.
This was pointed out for ingesters in #6492 (comment), and distributors had the same bug.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]