store-gateway: Move eager loading to BucketStore #8602
Conversation
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/move-eager-loading-to-bucket-store
branch
from
d4c5ba0
to
00e664c
Compare
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/snapshotter-service
branch
from
fb8d68e
to
9725277
Compare
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/move-eager-loading-to-bucket-store
branch
from
00e664c
to
4a2976a
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Move eager loading from LazyBinaryReader to BucketStore Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Use correct eager loading config Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/move-eager-loading-to-bucket-store
branch
from
4a2976a
to
e4828c7
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
|
this is now ready for a review |
pkg/storegateway/bucket.go
Outdated
| previouslyLoadedBlocks, err := indexheader.RestoreLoadedBlocks(s.dir) | ||
| if err != nil { | ||
| level.Warn(s.logger).Log( | ||
| "msg", "loading the list of index-headers from snapshot file failed; not eagerly loading index-headers for tenant", | ||
| "dir", s.dir, | ||
| "err", err, | ||
| ) | ||
| // Don't fail initialization. If eager loading doesn't happen, then we will load index-headers lazily. | ||
| // Lazy loading which is slower, but not worth failing startup for. | ||
| } |
There was a problem hiding this comment.
How about we move this chunk inside s.loadBlocks (and maybe rename this method to s.restorePreviousBlocks)? We don't need to do it if EagerLoadingStartupEnabled is disabled. And also this will make the piece about "not failing the initialization" less awkward (IMHO).
There was a problem hiding this comment.
after we run syncBlocks the snapshotter will be running, so there's a theoretical race between the snapshotter and reading the snapshot from before the shutdown. It's not obvious, so I now added a comment explaining this and moved this whole thing into a function.
But if you have ideas for how to make this better I can do it
There was a problem hiding this comment.
It makes more sense now, thanks for explaining. I'm ok to stick to the current version. I think, the comments you've added makes it clear 👍
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
deleted the
dimitar/st-gw/move-eager-loading-to-bucket-store
branch
This is the third PR for #8389
What this PR does
This PR does one major change: Move eager loading from
ReaderPooltoBucketStore.The problem with keeping eager loading in the
ReaderPoolis that it needs to have the list of previously loaded blocks injected. This is hard if we want to move the snapshotter and index reader pool to being services.The PR also slightly changes the
Snapshotterto receive its dependency (BlocksLoaderwhich has the list of currently loaded blocks) via the constructor as opposed to via each snapshot iteration. This is now possible because there is no circular dependency between theSnapshotterand theReaderPool(the snapshotter needed to pool to know what's loaded; the pool needed the snapshotter to know what to eager load).Which issue(s) this PR fixes or relates to
related to #8389
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.