fix(file source): make FinalizerSet optionally handle shutdown signals #16928
Conversation
✅ Deploy Preview for vector-project canceled.
|
✅ Deploy Preview for vrl-playground ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
github-actions
bot
added
the
domain: sources
Regression Detector ResultsRun ID: 9edb3990-96a9-432a-9eb8-4d5c8f6a292e ExplanationA regression test is an integrated performance test for The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
| let (finalizer, mut stream) = OrderedFinalizer::new(ShutdownSignal::noop()); | ||
| let (finalizer, mut stream) = OrderedFinalizer::new(None); |
There was a problem hiding this comment.
Should this not be Some here too, at least for now? The description says only the file source is being modified.
There was a problem hiding this comment.
Oh, yes I should have mentioned this case too. If I understand correctly, ShutdownSignal::noop() gives back a ShutdownSignal that is not attached to the global shutdown trigger, and can't be triggered externally. In this case, I believe this "noop" ShutdownSignal should be functionally equivalent to passing None, since it can't be signaled until the finalizer stream that owns it is dropped?
There was a problem hiding this comment.
FWIW, after adding a unit test for the file source, I ran those tests locally using Some(ShutdownSignal::noop()) in the file source ack stream, and that also fixes the bug - evidence that it is indeed equivalent to passing None.
That said, if there is a concern with including this change here I'm happy to revert this line for now.
bruceg
added
type: bug
Regression Detector ResultsRun ID: 1cc8752d-d53e-47d1-a760-00103589b180 ExplanationA regression test is an integrated performance test for The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
jches
requested review from
bruceg
and removed request for
StephenWakely,
spencergilbert and
davidhuie-dd
Regression Detector ResultsRun ID: 1c121c44-576a-4be1-9ffb-c5d6182c50b6 ExplanationA regression test is an integrated performance test for The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values mean that baseline is faster, positive comparison. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. Changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%:
Fine details of change detection per experiment.
|
|
Thanks for your work on this @jches! |
|
Cheers, thanks for the review & feedback. I should have another coming soon to fix this for the kafka source. |
This fixes a problem where, when Vector receives a shutdown signal, sources may not process acknowledgements for messages that have been processed, because the
FinalizerSetimplementation stops processing entries as soon as the shutdown signal is received.In principle,
FinalizerSetshould not need to listen for shutdown signals, and should instead rely on the source closing sender side of the "new acknowledgement entry" channel on shutdown. However, it may currently be the case that some sources do not close that channel as expected, and hence unintentionally rely on theFinalizerSetending to exit properly. Since removing shutdown signal handling fromFinalizerSetmay cause some sources to block indefinitely, this change makes theShutdownSignalparameter optional, and most sources using it pass in aSometo preserve their current behavior.Only the
filesource is updated here to passNone, opting out of shutdown handling by the acknowledgement stream, and thefile-sourcecode is updated to ensure the acknowledgement sender channel is closed at shutdown.This is intended to update/replace (in part) a previous PR related to this bug: #14846. See also discussion in #16827 for more details.