Fix TestLabelNames_Cancelled flakyness #8434
Conversation
Signed-off-by: Marco Pracucci <marco@pracucci.com>
| func (s *BucketStore) RemoveBlocksAndClose() error { | ||
| return s.removeBlocksAndClose(context.Background(), false) | ||
| } | ||
|
|
||
| // RemoveBlocksCloseAndWait remove all blocks from local disk, releases all resources associated with the BucketStore | ||
| // and waits until all dependencies have been stopped. | ||
| func (s *BucketStore) RemoveBlocksCloseAndWait(ctx context.Context) error { | ||
| return s.removeBlocksAndClose(ctx, true) | ||
| } |
There was a problem hiding this comment.
we should be having this synchronous in all cases. If the snapshotter shuts down async, then it can create a file after the tenant directory has been cleaned up. Previously RemoveBlocksAndClose has been synchronous anyways.
having said that, it's also ok to not necessarily propagate the context all the way to RemoveBlocksCloseAndWait. This will be done soon with #8389
There was a problem hiding this comment.
having said that, it's also ok to not necessarily propagate the context all the way to RemoveBlocksCloseAndWait.
I think we need to, actually. At least for a case when the first syncBlocks fails (or was cancelled), the Snapshotter won't start. In this case, the call to Snapshotter.Stop will block, waiting on an empty channel.
With the above at hand, I wonder if it would be simpler to replace Snapshotter.stopped channel with a WaitGroup:
func Start() {
wg.Add(1)
go func() {
defer wg.Done()
}
}
func Stop() {
close(stop)
wg.Wait()
}Although, the above it a (nit), given the plans for #8389
There was a problem hiding this comment.
ok, let's keep the current implementation. This is a nightmare to navigate
What this PR does
TestLabelNames_Cancelledis flaky because of this. To fix it, I propose to wait until snapshotter is effectively stopped in tests.I start from the assumption that
Snapshotter.Start()won't be called more than once, otherwise it panics. ButSnapshotterdidn't support multiple start-stop cycles even before (because it would panic inSnapshotter.Stop().Which issue(s) this PR fixes or relates to
Fixes #8428
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.