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

mimirpb: export IsClientErrorCause() function #8122

Merged
merged 1 commit into from
May 13, 2024
Merged

Conversation

duricanikolic
Copy link
Contributor

What this PR does

This PR exports mimirpb.IsClientErrorCause() function, that distinguishes between mimirpb.ErrorCauses corresponding to client and server errors. Currently, mimirpb.BAD_DATA is the only mimirpb.ErrorCause corresponding to client errors.

This PR makes no semantical change, it just exports the error cause check already used within mimirpb.IsClientError().

Checklist

  • [na] Tests updated.
  • [na] Documentation added.
  • [na] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [na] about-versioning.md updated with experimental features.

@duricanikolic duricanikolic self-assigned this May 13, 2024
@duricanikolic duricanikolic requested a review from a team as a code owner May 13, 2024 11:36
@duricanikolic duricanikolic requested a review from replay May 13, 2024 11:37
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

Approved because this doesn't change any logic.
I still think it's weird that for example METHOD_NOT_ALLOWED is defined to not be a client error here, but that's a separate discussion outside the scope of this PR.

@duricanikolic duricanikolic merged commit c9423a6 into main May 13, 2024
29 checks passed
@duricanikolic duricanikolic deleted the yuri/fix-errors branch May 13, 2024 12:26
@narqo
Copy link
Contributor

narqo commented May 13, 2024

I still think it's weird that for example METHOD_NOT_ALLOWED is defined to not be a client error

In #7618 we had a somewhere long discussion on if ours gRPC METHOD_NOT_ALLOWED should be mapped to HTTP 405 or 501. We landed up with the latter mainly because of how this specific error was used in Mimir's ingest-storage.

@colega
Copy link
Contributor

colega commented May 14, 2024

Sounds like the problem is that METHOD_NOT_ALLOWED is a wrong name for that status code (because it's something that lives in a similar domain as HTTP, but has a completely different meaning from 405 Method Not Allowed).

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