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.

Sign up for GitHub

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

Jump to bottom

Fix chunks flushing bugs #2778

Merged
merged 7 commits into from Jul 1, 2020
Merged

Fix chunks flushing bugs #2778

merged 7 commits into from Jul 1, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jun 24, 2020 •
edited

What this PR does: There were few bugs in chunks flushing code that this PR tries to fix:

  • Panic while flushing chunks #2743
  • Samples added during immediate flush were discarded
  • If removal of old chunks happens during flush, incorrect chunks are marked as flushed (this can now happen since this PR keeps immediate-flushed chunks in memory)

In addition to that, this PR also:

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

Checklist

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

(Notify @gouthamve @bboreham as chunk experts)

@pstibrany pstibrany mentioned this pull request Jun 24, 2020
Closed
@bboreham
Copy link
Contributor

I wonder if it would be possible to fix #2287 at the same time?
Maybe if we move the point that the metric is computed to after we know what happened to the chunk, we can get away without adding a new metric?

pracucci
pracucci approved these changes Jun 25, 2020
View reviewed changes
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.

Excellent job, LGTM. I just left a couple of questions.

pkg/ingester/flush.go Show resolved Hide resolved
pkg/ingester/flush.go Show resolved Hide resolved
bboreham
bboreham reviewed Jun 25, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@pstibrany
Copy link
Contributor Author

I wonder if it would be possible to fix #2287 at the same time?
Maybe if we move the point that the metric is computed to after we know what happened to the chunk, we can get away without adding a new metric?

I will take a look at it before merging.

@pstibrany pstibrany mentioned this pull request Jun 26, 2020
Closed
@pstibrany
Copy link
Contributor Author

I wonder if it would be possible to fix #2287 at the same time?
Maybe if we move the point that the metric is computed to after we know what happened to the chunk, we can get away without adding a new metric?

I will take a look at it before merging.

I've sent separate PR #2802 for issue #2287.

@pstibrany
Add test case demonstrating bug and panic from issue 2743.
fb5f0b6 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Several fixes:
1f7f9d7 
- if there are no unflushed chunks, shouldFlushSeries in immediate mode
  returns noFlush
- flushUserSeries works with a copy of chunk descriptors slice (not copy
  of descriptors themselves, since slice has a pointers to them).
  This helps to prevent panic in issue 2743
- Don't remove entire series from memory on immediate flush.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Added CHANGELOG.md
1c8e5bb 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Added cortex_ingester_flush_series_in_progress metric and fix integra…
121beca 
…tion test.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Update CHANGELOG.md wording.
8b557bc 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Fix misleading messages.
9943ee0 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Copy link
Contributor Author

I will merge this tomorrow, if there are no objections.

@pstibrany pstibrany merged commit bd53ab3 into cortexproject:master Jul 1, 2020
@pracucci pracucci mentioned this pull request Jul 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bboreham bboreham bboreham left review comments

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/flush handler doesn't wait for flush to finish Panic while flushing chunks
3 participants
@pstibrany @bboreham @pracucci