[dbnode] Introduce a field for RPC error code and Resource Exhausted error type #3005
Conversation
| } else if xerrors.IsInvalidParams(err) { | ||
| } else if xerrors.IsInvalidParams(err) || xerrors.IsResourceExhausted(err) { |
There was a problem hiding this comment.
There are quite a few cases in this PR grouping invalid param with resource exhausted errors does make sense, e.g.:
- returning same status code (404 in this case);
- incrementing the same metric.
In such cases I wanted to limit the amount of changes in a single PR and I erred on maintaining externally observable behavior (status codes, published metrics) as it was up until now. This would be fixed in upcoming commits.
| // NB: To maintain better backwards compatibility, using BAD_REQUEST error type coupled with | ||
| // RESOURCE_EXHAUSTED error code. After a reasonable amount of time this should be switched to | ||
| // RESOURCE_EXHAUSTED error code (added after M3 release v1.0.0) | ||
| return newError(rpc.ErrorType_BAD_REQUEST, rpc.ErrorCode_RESOURCE_EXHAUSTED, err) |
There was a problem hiding this comment.
This should introduce the protocol changes in a backwards compatible way. Older clients, which are not aware about Resource Exhausted error type, will fallback to current Bad Request behavior. On the other hand, newer client would know about Resource Exhausted errors, but the server would keep sending Bad Request until it's upgraded.
| @@ -192,7 +192,7 @@ func (q *lookbackLimit) exceeded() error { | |||
| func (q *lookbackLimit) checkLimit(recent int64) error { | |||
| if q.options.Limit > 0 && recent > q.options.Limit { | |||
| q.metrics.exceeded.Inc(1) | |||
| return xerrors.NewInvalidParamsError(fmt.Errorf( | |||
| return xerrors.NewResourceExhaustedError(fmt.Errorf( | |||
There was a problem hiding this comment.
Using Resource Exhausted for query limits
| func IsResourceExhausted(err error) bool { | ||
| return GetInnerResourceExhaustedError(err) != nil | ||
| } |
There was a problem hiding this comment.
NB: these methods might behave in a confusing way. E.g. NewResourceExhaustedError(NewInvalidParamError(err)) could be treated as both depending on which of IsResourceExhausted() or IsInvalidParam() was called first.
vpranckaitis
changed the title
vpranckaitis
requested review from
robskillington,
arnikola,
gediminasgu,
linasm and
soundvibe
| return tterrors.NewBadRequestError(err) | ||
| } | ||
| return tterrors.NewInternalError(err) | ||
| } |
There was a problem hiding this comment.
Can we put this in the convert package?
In fact this method already exists, we would just need to add it:
https://github.com/m3db/m3/blob/master/src/dbnode/network/server/tchannelthrift/convert/convert.go#L187:L197
Codecov Report
@@ Coverage Diff @@
## master #3005 +/- ##
========================================
+ Coverage 70.5% 72.3% +1.7%
========================================
Files 1078 1078
Lines 99840 99459 -381
========================================
+ Hits 70430 71916 +1486
+ Misses 24287 22537 -1750
+ Partials 5123 5006 -117
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
vpranckaitis
changed the title
| @@ -54,6 +54,35 @@ func IsBadRequestError(err error) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // IsResourceExhaustedError determines if the error is a resource exhausted error. | |||
| func IsResourceExhaustedError(err error) bool { | |||
There was a problem hiding this comment.
nit: Call this IsResourceExhaustedErrorCode to match the fact that it's matching on ErrorCode not ErrorType?
| @@ -32,9 +32,15 @@ enum ErrorType { | |||
| BAD_REQUEST | |||
| } | |||
|
|
|||
| enum ErrorCode { | |||
There was a problem hiding this comment.
nit: Let's use ErrorFlags instead of ErrorCode as we arrived at during our catchup? Will map the naming of the error value to how we would like to use them if flags&RESOURCE_EXHAUSTED { ... }.
vpranckaitis
removed request for
gediminasgu,
soundvibe,
linasm and
arnikola
|
Redone in #3017 |
What this PR does / why we need it:
Introduces
Resource Exhaustederror type and uses it forquery limit exceedederrors. This includes addingcodefield torpc.Errorand introducing newxerrors.NewResourceExhaustedError().Special notes for your reviewer:
There are quite a few changes, it might be a bit more convenient to review the them commit-by-commit.
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: