store-gateway: Make Snapshotter a service #8552
Conversation
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/snapshotter-service
branch
from
891c0df
to
f984b9d
Compare
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/snapshotter-service
branch
from
f984b9d
to
cad7f4e
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
force-pushed
the
dimitar/st-gw/snapshotter-service
branch
from
fb8d68e
to
9725277
Compare
|
this is now ready for review |
| } | ||
|
|
||
| func (s *Snapshotter) PersistLoadedBlocks(bl blocksLoader) error { | ||
| func (s *Snapshotter) PersistLoadedBlocks(bl BlocksLoader) error { |
There was a problem hiding this comment.
Let's remove the bl argument here and use s.bl that the snapshotter now owns. I can't think of a use-case where someone would want to call the method with a different instance of BlocksLoader.
There was a problem hiding this comment.
this was actually done in the next PR https://github.com/grafana/mimir/pull/8602/files#diff-5b4f5a512e5270de93a5784bedbb0e2b6f9cd3ea3859b072fa25da3651b97116R64-R69
do you mind I don't do it in this PR to save myself from resolving conflicts 🙏 😄
pkg/storegateway/bucket.go
Outdated
| s.snapshotter = indexheader.NewSnapshotter(s.logger, snapConfig, s.indexReaderPool) | ||
| } else { | ||
| s.snapshotter = noopShapshotter{services.NewIdleService(nil, nil)} | ||
| } | ||
| s.indexReaderPool = indexheader.NewReaderPool(s.logger, bucketStoreConfig.IndexHeader, s.lazyLoadingGate, metrics.indexHeaderReaderMetrics, lazyLoadedBlocks) | ||
| s.indexReaderPool = indexheader.NewReaderPool(s.logger, bucketStoreConfig.IndexHeader, s.lazyLoadingGate, metrics.indexHeaderReaderMetrics, s.snapshotter.RestoreLoadedBlocks()) |
There was a problem hiding this comment.
Oh, now I recall this "chicken and an egg", where the Snapshotter takes IndexReader, while the IndexReader needs Snapshotter.
Looking at it now, the Snapshotter.RestoreLoadedBlocks doesn't have to be a method — I made it so mainly to be symmetric to its Snapshotter.PersistLoadedBlocks. How about we make the former a function instead of a (static) method and reorder these constructor calls here to unwind it? This will make it less surprising for a reader.
There was a problem hiding this comment.
i actually found a bug with the current initialization - we pass a nil to indexheader.NewSnapshotter. It's fixed in the later PR, but it's still a bug here :(
I think I did what you suggested.
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>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
|
thanks for the review @narqo. Most of this had gone wrong when I tried splitting my changes into multiple PRs. PTAL 🙏 |
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
What this PR does
This is the second PR for #8389.
The changes here are:
indexheader.Snapshotteris now aservices.Servicewith a simple timing loopChecklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.