feat(rebaser): change quiescent shutdown to reduce missed activity #4707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change alters the logic that helps a change set "process" task to shut down when no Rebaser requests have been seen over our `quiescent_period`. Prior to this change there was a shutdown window period where the `ChangeSetProcessorTask` would not be looking for new Rebaser requests to process while waiting for the `SerialDvuTask` to end. As a hedge against this scenario the process task handler checks the change set subject just before ending to ensure that if there's at least one request message that we don't ack/delete the task. In this altered version of a quiescent shutdown we notice the quiet period as before in the Rebaser requests subscription stream. However, now a `quiesced_notify` `tokio::sync::Notify` is fired to signal the `SerialDvuTask`. Then the `ChangeSetProcessorTask` continues to process any further requests that may show up (remember that after running a "dvu" job, another Rebaser request is often submitted). Meanwhile in the `SerialDvuTask`, it will continue to run "dvu" jobs as long as the `run_dvu_notify` has been set (in effect "draining" any pending runs), and only then will check to see if the `quiesced_notify` has been set. If it has, then it will cancel the `quiesced_token` which cause `SerialDvuTask` to return with an `Ok(Shutdown::Quiesced)` and that same `CancellationToken` will cause the Naxum app in `ChangeSetProcessorTask` to be gracefully shut down. With these changes, the one or two remaining "dvu" jobs will not cause the process task to stop processing further Rebaser requests. For example, let's assuming that the last 2 "dvu" jobs take 8 minutes each. That means that the process task is in a quiescent shutdown for up to the next 8 * 2 = 16 minutes, during which time any further Rebaser requests will also be processed (whereas they may not have been prior to this change). Signed-off-by: Fletcher Nichol <[email protected]>
|
/try [using pr as a dummy for CI testing changes] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change alters the logic that helps a change set "process" task to shut down when no Rebaser requests have been seen over our
quiescent_period. Prior to this change there was a shutdown window period where theChangeSetProcessorTaskwould not be looking for new Rebaser requests to process while waiting for theSerialDvuTaskto end. As a hedge against this scenario the process task handler checks the change set subject just before ending to ensure that if there's at least one request message that we don't ack/delete the task.In this altered version of a quiescent shutdown we notice the quiet period as before in the Rebaser requests subscription stream. However, now a
quiesced_notifytokio::sync::Notifyis fired to signal theSerialDvuTask. Then theChangeSetProcessorTaskcontinues to process any further requests that may show up (remember that after running a "dvu" job, another Rebaser request is often submitted). Meanwhile in theSerialDvuTask, it will continue to run "dvu" jobs as long as therun_dvu_notifyhas been set (in effect "draining" any pending runs), and only then will check to see if thequiesced_notifyhas been set. If it has, then it will cancel thequiesced_tokenwhich causeSerialDvuTaskto return with anOk(Shutdown::Quiesced)and that sameCancellationTokenwill cause the Naxum app inChangeSetProcessorTaskto be gracefully shut down.With these changes, the one or two remaining "dvu" jobs will not cause the process task to stop processing further Rebaser requests. For example, let's assuming that the last 2 "dvu" jobs take 8 minutes each. That means that the process task is in a quiescent shutdown for up to the next 8 * 2 = 16 minutes, during which time any further Rebaser requests will also be processed (whereas they may not have been prior to this change).