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.

Sign up for GitHub

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

ingest storage: proper shutdown of partitionCommitter #9436

Merged
merged 1 commit into from
Oct 1, 2024
Merged

ingest storage: proper shutdown of partitionCommitter #9436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/storage/ingest/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ func (r *PartitionReader) start(ctx context.Context) (returnErr error) {
if err != nil {
return errors.Wrap(err, "creating service manager")
}
err = services.StartManagerAndAwaitHealthy(ctx, r.dependencies)
// Use context.Background() because we want to stop all dependencies when the PartitionReader stops
// instead of stopping them when ctx is cancelled and while the PartitionReader is still running.
err = services.StartManagerAndAwaitHealthy(context.Background(), r.dependencies)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the tradeoff here is that if we get interrupted during startup, we wouldn't respect that and we'd wait for the full startup to finish

not sure if I should try to solve that (maybe with something like context.AfterFunc())

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the PartitionReader gets terminated (context canceled) while running PartitionReader.start(), do we evern call stopDependencies() at all? If I remember correctly, PartitionReader.stop() will get called only if PartitionReader.start() has successfully terminated, that is if it has returned nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we take care of it a few lines above

mimir/pkg/storage/ingest/reader.go

Lines 105 to 110 in 01d32ac

// Stop dependencies if the start() fails.
defer func() {
if returnErr != nil {
_ = r.stopDependencies()
}
}()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, I missed it. LGTM Then.

if err != nil {
return errors.Wrap(err, "starting service manager")
}
Expand Down
Loading