Skip to content

Don't return 500's for KV CAS calls if the CAS returned an HTTP status code. #1798

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

Merged
merged 1 commit into from
Nov 10, 2019
Merged

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented Nov 8, 2019

CAS calls to the KV store can return HTTP status codes since the addition of the HA tracking code.

If the return error of the CAS has an HTTP status code, we should return that. This will resolve the issue of the KV metrics showing 500's for all writes from the non-elected Prometheus replicas.

Signed-off-by: Callum Styan [email protected]

@tomwilkie

addition of the HA tracking code. If the return error of the CAS
has an HTTP status code, we should return that. This will resolve
the issue of the KV metrics showing 500's for all writes from the
non-elected Prometheus replicas.

Signed-off-by: Callum Styan <[email protected]>
@cstyan cstyan changed the title DonCAS calls to the KV store can return HTTP status codes since the Don't return 500's for KV CAS calls if the CAS returned an HTTP status code. Nov 8, 2019
if err == nil {
return "200"
}
if resp, ok := httpgrpc.HTTPResponseFromError(err); ok {
Copy link
Contributor

@pstibrany pstibrany Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works for errors from GRPC layer. I don't think etcd or consul use GRPC. Update: etcd v3 does!

@cstyan
Copy link
Contributor Author

cstyan commented Nov 8, 2019

@pstibrany This change is to allow us to differentiate between actual errors and short circuit exits from CAS calls within the HA tracking code: https://github.com/cortexproject/cortex/blob/master/pkg/distributor/ha_tracker.go#L237-L239
https://github.com/cortexproject/cortex/blob/master/pkg/distributor/ha_tracker.go#L220-L225

@tomwilkie tomwilkie requested a review from gouthamve November 9, 2019 12:32
@tomwilkie tomwilkie merged commit ef99cda into cortexproject:master Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants