-
Notifications
You must be signed in to change notification settings - Fork 524
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
Get rid of httpgrpc on distributor's write path #6377
Conversation
f3b329b
to
30140f7
Compare
pkg/distributor/errors.go
Outdated
// requestRateLimitedError implements the distributorError interface. | ||
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause { |
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.
Opinionated nit, but this says the same, plus it's also checked by the compiler and can't get outdated.
// requestRateLimitedError implements the distributorError interface. | |
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause { | |
var _ distributorError = requestRateLimitedError{} | |
func (e requestRateLimitedError) errorCause() mimirpb.ErrorCause { |
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 have implemented the required change.
pkg/mimirpb/mimir.proto
Outdated
@@ -34,6 +34,18 @@ message WriteRequest { | |||
|
|||
message WriteResponse {} | |||
|
|||
enum ErrorCause { | |||
REPLICAS_DID_NOT_MATCH = 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.
Can we add an INVALID = 0
const here? That would prevent the code from silently being interpreted as REPLICAS_DID_NOT_MATCH
if there's a bug somewhere and cause isn't set.
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.
Sure, I can do it.
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
b603ae2
to
7e6e38d
Compare
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
return mimirpb.REPLICAS_DID_NOT_MATCH | ||
} | ||
|
||
var _ distributorError = replicasDidNotMatchError{} |
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.
What are these statements for? is that to verify that the type satisfies the interface?
It would be useful (for me) if there was a comment what this does.
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.
Hi @replay,
Exactly, these statements are here for verifying that the type satisfies the interface.
I was asked by @colega (here) to add them because this way the compiler complains if we by mistake forget to implement some methods of an interface.
I agree that adding a comment explaining that would be beneficial and will add 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.
I'm okay with having a comment, although this is a standard Golang statement.
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
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 looks good to me, thx
What this PR does
This PR modifies the existing implementation of
Distributor.Push()
by returning only errors withgRPC
codes coming from the distributor. The only exceptions are the errors returned by the ingester, which can still contain HTTP status codes. This will be fixed in one of the following PRs.Proto messages
WriteErrorDetails
andErrorCause
field have been added to themimirpb
package.The purpose of
WriteErrorDetails
is to enrich distributor'sgRPC
errors with additional details, which closely explain the nature of the error itself. Possible values ofErrorCause
s are analog to the distributor error types.The following table shows the mapping between the error types encountered during
Distributor.Push()
and the correspondinggRPC
codes:validationError
codes.FailedPrecondition
mimirpb.VALIDATION
ingestionRateLimitedError
codes.ResourceExhausted
mimirpb.INGESTION_RATE_LIMITED
requestRateLimitedError
code.Unavailable
orcodes.ResourceExhausted
mimirpb.REQUEST_RATE_LIMITED
replicasDidNotMatchError
codes.AlreadyExists
mimirpb.REPLICAS_DID_NOT_MATCH
tooManyClustersError
codes.FailedPrecondition
mimirpb.TOO_MANY_CLUSTERS
codes.Internal
The table above does NOT include the errors returned by the ingester which contain either HTTP status codes 400 and 503 or gRPC codes: these errors are propagated in the original form. This will be improved in one of the following PRs.
Which issue(s) this PR fixes or relates to
Part of #6008
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]