Skip to content

Removed ingesters blocks transfer support #2996

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

Merged

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Aug 7, 2020

What this PR does:
As discussed in #2966, this PR removes the blocks transfer support from ingesters.

/cc @codesome

Which issue(s) this PR fixes:
Fixes #2966 and #1828.

Checklist

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

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.

Looks good! Thanks for this change.

Similarly to the blocks storage, when Cortex is running the chunks storage with WAL enabled, it requires ingesters to run with a persistent disk where the WAL is stored (eg. a StatefulSet when deployed on Kubernetes).

During a rolling update, the leaving ingester closes the WAL, synchronize the data to disk (`fsync`) and releases the disk resources.
The new ingester, which is expected to reuse the same disk of the leaving one, will replay the WAL on startup in order to load back in memory the time series that have not been flushed to the storage yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

It actually loads all data into memory, even flushed samples. WAL doesn't preserve "flushed" flag, so data will be possibly flushed twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I'm not sure we should mention the "data will be possibly flushed twice" here, given this page is focused on rolling updates. Maybe it's something we should improve in the "Ingesters with WAL" page. WDYT?

In the meanwhile, I've rephrased it inhttps://github.com//pull/2996/commits/caa7dd453b918cd0674cbc52d999490e3ceebe0b.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In that change you have introduce "checkpoint" term, with no explanation. I think we can be bit more vague and just say that ingester reloads data from WAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, done.

@pracucci pracucci force-pushed the remove-blocks-transfer-support branch from 7f2a9c1 to 37dade1 Compare August 10, 2020 09:35
@pracucci pracucci merged commit 4be2876 into cortexproject:master Aug 10, 2020
@pracucci pracucci deleted the remove-blocks-transfer-support branch August 10, 2020 12:26
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.

Proposal: Remove support for TSDB blocks transfer between ingesters
2 participants