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

Improved Ingester out-of-order error for faster troubleshooting #1008

Merged
merged 7 commits into from
Sep 17, 2019
Merged

Improved Ingester out-of-order error for faster troubleshooting #1008

merged 7 commits into from
Sep 17, 2019

Conversation

wardbekker
Copy link
Member

What this PR does / why we need it:
See #963

Which issue(s) this PR fixes:
Fixes #963, although not returning a JSON struct yet, that could be a follow up improvement

Special notes for your reviewer:
First pull request to Loki, love to get your feedback!

Checklist

  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Sep 11, 2019

CLA assistant check
All committers have signed the CLA.

@wardbekker wardbekker changed the title #963: Improved Ingester out-of-order error for faster troubleshooting Improved Ingester out-of-order error for faster troubleshooting Sep 11, 2019
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM! I do have concerns about how it changes the behavior of the code when we received errors that aren't out-of-order errors.

pkg/ingester/stream.go Outdated Show resolved Hide resolved
pkg/ingester/stream.go Outdated Show resolved Hide resolved
@slim-bean
Copy link
Collaborator

My only big concern with merging this, is that I think we need to figure out the return first. That is I don't want to merge this now then convert it to JSON output later.

@slim-bean
Copy link
Collaborator

This kind of then opens a bigger can of worms around what our API error handling looks like/should look like

@wardbekker
Copy link
Member Author

My only big concern with merging this, is that I think we need to figure out the return first. That is I don't want to merge this now then convert it to JSON output later.

no worries. will work on a JSON return within this pull request

@slim-bean
Copy link
Collaborator

Hold off just a bit @wardbekker we were discussing a little in slack, but I think we need a strategy for error return types in general. Don't want you to head down a road until we are sure what direction we want to go with this

@rfratto
Copy link
Member

rfratto commented Sep 17, 2019

This LGTM, we're going to tackle changes to error return types in a future PR, and this is a good improvement to have in the interrim.

@rfratto rfratto merged commit 3b96510 into grafana:master Sep 17, 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.

Improve Ingester out-of-order error for faster troubleshooting
4 participants