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

Fail ingestion when a flush is going on #3422

Merged
merged 7 commits into from
Nov 3, 2020

Conversation

codesome
Copy link
Contributor

@codesome codesome commented Oct 29, 2020

What this PR does:

Fails a push request with a 5xx when there is an ongoing flush. This is according to the idea proposed in this comment #3282 (comment)

Which issue(s) this PR fixes:
Fixes #3282 #3348

Checklist

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

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @codesome about this! We had an offline discussion about a race condition. Ganesh mentioned he will work on this tomorrow. Thanks!

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, but with a little fix, thanks!

pkg/ingester/ingester_v2.go Show resolved Hide resolved
@pracucci pracucci requested a review from pstibrany October 30, 2020 09:56
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@pracucci
Copy link
Contributor

pracucci commented Nov 2, 2020

@pstibrany Me and @codesome are discussion a potentially better way to handle this. Please don't merge it you approve it.

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Ganesh!

pkg/ingester/ingester_v2.go Outdated Show resolved Hide resolved
@pracucci
Copy link
Contributor

pracucci commented Nov 2, 2020

@pstibrany Me and @codesome are discussion a potentially better way to handle this. Please don't merge it you approve it.

@pstibrany Ignore this. After further discussion, Ganesh has no bandwidth to experiment alternative solutions (eg. disable blocks compaction at all in TSDB and enable overlapping blocks in TSB, making sure query performances don't degrade significantly on overlapping blocks), so I think we can get this PR in and then we may reconsider this approach if/when Ganesh will have bandwidth.

So we can merge if you approve, but let's wait for Ganesh adding the test suggested by Richard first.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 3, 2020
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Very good job. LGTM, modulo a couple of nits. Thanks!

pkg/ingester/ingester_v2_test.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_v2_test.go Outdated Show resolved Hide resolved
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thanks. I think there may be race errors in the test, but otherwise LGTM.

pkg/ingester/ingester_v2_test.go Show resolved Hide resolved
pkg/ingester/ingester_v2_test.go Show resolved Hide resolved
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential Prometheus memory leak in ingesters
4 participants