ingester: Enforce limits for Push requests coming via ingest.Store #7621
Conversation
| type ingesterPushReceiver interface { | ||
| StartPushRequest(requestSize int64) error | ||
| FinishPushRequest(requestSize int64) | ||
| StartPushRequest(ctx context.Context, requestSize int64) (context.Context, error) | ||
| FinishPushRequest(ctx context.Context) | ||
| } |
There was a problem hiding this comment.
I think a nice side effect of these changes, is that we don't need two interfaces ingesterPushReceiver and distributorPushReceiver any more. Here I kept them in place, but want to refactor this chunk further later.
narqo
force-pushed
the
ingest-store-disable-grpc-inflight-limiter
branch
from
5a20206 to
276820b
Compare
There was a problem hiding this comment.
I think there is a bug. When ingest store is enabled, but Push request arrives via gRPC (which we keep as an option, and may need during migrating cells), then i.StartPushRequest is correctly called from both gRPC method checker and from ingester, and it only "starts" the request once.
However both places also "finish" the request by calling FinishPushRequest or defer i.finishPushRequest(reqSize) respectively. And this will decrease inflightPushRequests twice.
In distributor we solve this by having pushHandlerPerformsCleanup in the request state, and this determines which component is responsible for cleanup -- either distributor itself, or grpc method check.
narqo
force-pushed
the
ingest-store-disable-grpc-inflight-limiter
branch
from
276820b to
d1dcf6d
Compare
Very good point, thank you. I considered the case initially, but it slipped from the implementation. I should've added a test for the change from the beginning. PHAL |
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
ingest-store-disable-grpc-inflight-limiter
branch
from
d1dcf6d to
6f3b2bc
Compare
What this PR does
Following #7503
We noticed that ingest storage may slip from in-flight requests limits, after the deprecated
LimitInflightRequestsUsingGrpcMethodLimiter (default true)will be removed. These changes update the ingester in way, so the in-flight limits were enforced when ingest storage is used.The changes also prevent double-counting of the limits, during the migration period, when the ingesters will be deployed with enabled ingest storage, but the distributors will keep using gRPC API.
Which issue(s) this PR fixes or relates to
Fixes n/a
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.