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

Ingester block lifetime changes #628

Merged
merged 20 commits into from
Apr 9, 2021

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Apr 5, 2021

What this PR does:
This PR redoes some of the block lifecycle on the ingester to improve stability and startup times.

Previously the ingester would convert the WAL into a CompleteBlock which persisted the data on disk but kept the index and bloom filters in memory. After a restart the blocks would have to be recreated from the WALs since memory was lost, which caused both heavy performance hit on startup, but also an inability to query data that had been flushed but no longer in the WAL.

Now the ingester uses a local backend mounted under /wal/blocks. The WAL is flushed to this local backend first, which fully persists the data/index/bloom, etc. Then it is flushed to the remote backend. On restart any existing blocks are simply reloaded and the ingester resumes where it was. The flushed state of a local block is persisted with a timestamp in a dedicated flushed file.

No configuration or hosting changes are needed. The local backend is mounted under /wal/blocks/ by reusing the existing wal storage location, and it also uses the existing block encoding/downsampling/flushing options.

Which issue(s) this PR fixes:
#633

Checklist

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

@mdisibio mdisibio changed the title WIP - Ingester block lifetime changes Ingester block lifetime changes Apr 6, 2021
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Have some initial thoughts. It makes sense to me to move the "LocalBlock" and related functionality inside the /tempodb/wal package and provide methods for the ingester/instance to use.

modules/ingester/flush.go Outdated Show resolved Hide resolved
modules/ingester/flush.go Outdated Show resolved Hide resolved
modules/ingester/ingester.go Outdated Show resolved Hide resolved
tempodb/wal/wal.go Show resolved Hide resolved
modules/ingester/local_block.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Show resolved Hide resolved
tempodb/backend/local/local.go Outdated Show resolved Hide resolved
tempodb/encoding/backend_block.go Outdated Show resolved Hide resolved
tempodb/encoding/backend_block.go Outdated Show resolved Hide resolved
tempodb/encoding/streaming_block.go Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. This is definitely getting closer.

Some mild refactoring comments, but other than that I'm really liking where this is.

modules/ingester/instance.go Outdated Show resolved Hide resolved
modules/ingester/ingester.go Outdated Show resolved Hide resolved
tempodb/wal/append_block.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/wal/wal_test.go Outdated Show resolved Hide resolved
tempodb/wal/local_block.go Outdated Show resolved Hide resolved
tempodb/wal/local_block.go Outdated Show resolved Hide resolved
tempodb/encoding/backend_block.go Outdated Show resolved Hide resolved
@mdisibio mdisibio merged commit 93c378a into grafana:master Apr 9, 2021
@mdisibio mdisibio deleted the ingester-block-lifetime branch May 27, 2021 13:28
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