Reject series/write requests when max pending request limit is hit #114
Conversation
hczhu-db
force-pushed
the
load-shedding
branch
8 times, most recently
from
174dd33 to
d52aa34
Compare
hczhu-db
force-pushed
the
load-shedding
branch
from
d52aa34 to
2651ebf
Compare
| level.Info(logger).Log("msg", "set max pending gRPC write request in limiter", "max_pending_requests", conf.maxPendingGrpcWriteRequests) | ||
| } | ||
| limiter, err := receive.NewLimiterWithOptions( | ||
| conf.writeLimitsConfig, |
There was a problem hiding this comment.
consider reuse the writeLimitsConfig so less interface changes?
There was a problem hiding this comment.
I agree that it'd be ideal to have this config field in writeLimitsConfig, but DB pods don't load writeLimitsConfig at all. Pantheon-writer pods load that config. I'll have to keep it this way.
The interface is not changed. receive.NewLimiter() stays the same. I added another function receive.NewLimiterWithOptions().
There was a problem hiding this comment.
got it, that's fair, do you wanna add some unit tests for limiter to test the load shedding behavior?
jnyi
left a comment
jnyi
left a comment
There was a problem hiding this comment.
left a few comments, great job, i think we should also track remote write pending writes using limiter
| // Value 0 disables the feature. | ||
| maxPendingRequests int32 | ||
| pendingRequests atomic.Int32 | ||
| maxPendingRequestLimitHit prometheus.Counter |
There was a problem hiding this comment.
LGTM. Shall we consider adding an alert around this metrics?
There was a problem hiding this comment.
Definitely once the counter is there.
hczhu-db
force-pushed
the
load-shedding
branch
from
0398fdc to
cb958ba
Compare
|
|
||
| // RemoteWrite implements the gRPC remote write handler for storepb.WriteableStore. | ||
| func (h *Handler) RemoteWrite(ctx context.Context, r *storepb.WriteRequest) (*storepb.WriteResponse, error) { | ||
| if h.Limiter.ShouldRejectNewRequest() { |
There was a problem hiding this comment.
nice, let's add a unit test for this behavior?
There was a problem hiding this comment.
I can do that in a follow-up PR while testing it in Dev. It's quite tricky to write a unit test for such a feature. I want to see how useful it's in Dev before spending time on it.
jnyi
left a comment
jnyi
left a comment
There was a problem hiding this comment.
lgtm, thank you for doing this!
This is to prevent Receive server from begin overloaded
Tested in
dev-aws-eu-west-1