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

Make ingester errors use mimirpb.ErrorCause #6424

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Oct 18, 2023

What this PR does

This PR makes a further step towards the write path error handling improvement. Proto message mimirpb.ErrorCause (introduced in #6377) has been extended to cover, together with all possible causes of distributor errors, the causes of ingester errors as well. Its extended definition is:

enum ErrorCause {
  INVALID = 0;
  REPLICAS_DID_NOT_MATCH = 1;
  TOO_MANY_CLUSTERS = 2;
  BAD_DATA = 3;
  INGESTION_RATE_LIMITED = 4;
  REQUEST_RATE_LIMITED = 5;
  INSTANCE_LIMIT = 6;
  SERVICE_UNAVAILABLE = 7;
  TSDB_UNAVAILABLE = 8;
}

Values INSTANCE_LIMIT, SERVICE_UNAVAILABLE and TSDB_UNAVAILABLE have been added by this PR.

The definition of the ingesterError interface, implemented by all ingester-specific errors, has been changed from

type ingesterError interface {
  errorType() ingesterErrorType
}

to

type ingesterError interface {
  errorCause() mimirpb.ErrorCause
}

Its definition is now analogous to the definition of the the distributorError interface (introduced in #6377), which is implemented by all distributor-specific errors.

Moreover, the newErrorWithStatus() constructor has been updated in such a way that in a case of an ingesterError the resulting errorWithStatus contains also a status detail of type mimirpb.WriteErrorDetails corresponding to ingesterError.errorCause().

Which issue(s) this PR fixes or relates to

Part of #6008

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@duricanikolic duricanikolic requested a review from a team as a code owner October 18, 2023 14:21
@duricanikolic duricanikolic force-pushed the yuri/error-refactoring branch from c875b69 to ebefe13 Compare October 18, 2023 14:24
@duricanikolic duricanikolic requested a review from replay October 18, 2023 14:35
@duricanikolic duricanikolic force-pushed the yuri/error-refactoring branch from ebefe13 to 9e1af68 Compare October 18, 2023 14:35
func (e perUserSeriesLimitReachedError) errorType() ingesterErrorType {
return badData
func (e perUserSeriesLimitReachedError) errorCause() mimirpb.ErrorCause {
return mimirpb.BAD_DATA
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised the exceeding the user limit would result in bad data.
But since in this PR you don't want to change the behavior this is out of scope of the PR

Copy link
Contributor Author

@duricanikolic duricanikolic Oct 18, 2023

Choose a reason for hiding this comment

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

@replay We can always add an additional cause and change this case. Would it be more appropriate to mark it as TENANT_LIMIT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to not change any logic in this PR, but then maybe make that logic change in a separate PR, just to keep the changes distinct

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.

Looks good to me, thx!

@duricanikolic duricanikolic merged commit 89b0142 into main Oct 18, 2023
@duricanikolic duricanikolic deleted the yuri/error-refactoring branch October 18, 2023 16:27
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.

2 participants