Skip to content
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

Changed logic of handling RPC error with code Cancelled #5297

Merged

Conversation

vlad-diachenko
Copy link
Contributor

@vlad-diachenko vlad-diachenko commented Feb 2, 2022

What this PR does / why we need it:
changed logic to respond with '500 Internal Server Error' if RPC error with code 'Cancelled' happened. it's made to allow Loki to retry the request to the querier instead of returning '499 The request was canceled by the client.' back to the client.

Which issue(s) this PR fixes:
Fixes #4991

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

…r with code 'Cancelled' happened. it's made to allow loki to retry the request to the querier instead of returning '499 The request was cancelled by the client.' back to the client.
@dannykopping
Copy link
Contributor

Please make sure to include a section in the upgrade guide regarding this change; this is a change to an external interface, and could cause some unexpected behaviour for users.

@vlad-diachenko
Copy link
Contributor Author

Please make sure to include a section in the upgrade guide regarding this change; this is a change to an external interface, and could cause some unexpected behaviour for users.

done.

@cyriltovena
Copy link
Contributor

Please make sure to include a section in the upgrade guide regarding this change; this is a change to an external interface, and could cause some unexpected behaviour for users.

@dannykopping the bug was introduce before we release it. So we're fixing it back to how it was.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

docs/sources/upgrading/_index.md Outdated Show resolved Hide resolved
@dannykopping
Copy link
Contributor

Please make sure to include a section in the upgrade guide regarding this change; this is a change to an external interface, and could cause some unexpected behaviour for users.

@dannykopping the bug was introduce before we release it. So we're fixing it back to how it was.

Ah, my bad

@vlad-diachenko vlad-diachenko force-pushed the mapped_grpc_cancellation_error_to_internal_server_error branch from 05d22d4 to 95c58a8 Compare February 3, 2022 08:04
@vlad-diachenko
Copy link
Contributor Author

@cyriltovena @owen-d @dannykopping could somebody please merge it

@dannykopping dannykopping merged commit d4a4728 into main Feb 3, 2022
@dannykopping dannykopping deleted the mapped_grpc_cancellation_error_to_internal_server_error branch February 3, 2022 09:46
KMiller-Grafana pushed a commit to KMiller-Grafana/loki that referenced this pull request Feb 4, 2022
…r with code 'Cancelled' happened. it's made to allow loki to retry the request to the querier instead of returning '499 The request was cancelled by the client.' back to the client. (grafana#5297)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loki should not send "request was cancelled by the client" as an http response
4 participants