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

feat(spooler): Implement shutdown behavior in the spooler #3980

Merged
merged 20 commits into from
Sep 13, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Sep 3, 2024

This PR implements the shutdown behavior in the spooler which works in the following way:

  1. The shutdown service notifies the buffer service of shutting down.
  2. If the shutdown signal has a timeout, the buffer will try to spool to disk all the envelopes that it has in memory. Other implementations of the envelope stack will just drop envelopes.
  3. The spooling will take place in batch sizes of arbitrary size (100 for now), meaning that we will insert at most 100 elements per insertion. This is done so that we have more granular insertion in case of failures.
  4. In case the shutdown takes more than expected, the future will be cancelled and an error will be shown to the user.

Note that cancellation of the shutdown procedure might lead to data loss based on the progress w.r.t. the await points.

Closes: https://github.com/getsentry/team-ingest/issues/529

@iambriccardo
Copy link
Member Author

The big question I have for this PR, is which strategy should we employ to deal with the envelopes in the stacks? To me, the strategy depends on the implementation:

  • Memory -> we drain and send all envelopes to the project cache (within the time bounds, if any).
  • Disk -> we drain and persist all envelopes on disk (within the time bounds, if any).

In general, given the shutdown time bound, we still don't have to guarantee full persistence of envelopes but we can do a best effort.

@iambriccardo iambriccardo marked this pull request as ready for review September 7, 2024 08:46
@iambriccardo iambriccardo requested a review from a team as a code owner September 7, 2024 08:46
// Currently, we want to flush the buffer only for disk, since the in memory implementation
// tries to not do anything and pop as many elements as possible within the shutdown
// timeout.
let Self::Sqlite(buffer) = self else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to branch out here and not call any flush method on the memory impl, since I didn't want to make the flushing empty in the provider or stack, since it made no sense.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent choice!

nit: I would prefer an explicit match on all variants here. If we ever add, let's say, a CloudStorageEnvelopeBuffer, the compiler would then force us to implement the new match arm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's a good point. I initially implemented it that way, but felt it was too verbose. But your argument makes it a more valid approach! Will fix

// Currently, we want to flush the buffer only for disk, since the in memory implementation
// tries to not do anything and pop as many elements as possible within the shutdown
// timeout.
let Self::Sqlite(buffer) = self else {
Copy link
Member

Choose a reason for hiding this comment

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

Excellent choice!

nit: I would prefer an explicit match on all variants here. If we ever add, let's say, a CloudStorageEnvelopeBuffer, the compiler would then force us to implement the new match arm.

relay-server/src/services/buffer/mod.rs Outdated Show resolved Hide resolved
iambriccardo and others added 3 commits September 13, 2024 08:13
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
@iambriccardo iambriccardo merged commit 828b130 into master Sep 13, 2024
23 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/buffer-shutdown branch September 13, 2024 07:56
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