Skip to content

Restart extractor on config change #655

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

Open
Toshad wants to merge 7 commits into
base: master
Choose a base branch
from
+25 −1
Open

Restart extractor on config change #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f1ef43
Restart extractor on config change
Toshad Apr 9, 2026
b1b4b37
lint
Toshad Apr 9, 2026
fdaf294
fix test timeout
Toshad Apr 9, 2026
55fe623
Merge branch 'master' into config_restart
Toshad Apr 9, 2026
8982e83
Merge branch 'master' into config_restart
Hmnt39 Apr 10, 2026
d331f2b
Add never restart test
Toshad Apr 10, 2026
19b2c5d
fix test
Toshad Apr 10, 2026
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
6 changes: 6 additions & 0 deletions ExtractorUtils.Test/unit/Unstable/RuntimeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,19 @@ private ExtractorRuntimeBuilder<DummyConfig, DummyExtractor> CreateMockRuntimeBu
builder.ExternalServices = services;
builder.AddLogger = false;

// Reduce backoff times for faster tests
builder.BackoffBase = 50;

return builder;
}

[Fact(Timeout = 5000)]
public async Task TestRuntimeRestartNewConfig()
{
var builder = CreateMockRuntimeBuilder();
// Restart policy won't be the default "Always" in customer envs, but we should
// still restart on config change.
builder.RestartPolicy = ExtractorRestartPolicy.OnError;

using var evt = new ManualResetEventSlim(false);

Expand Down
20 changes: 19 additions & 1 deletion ExtractorUtils/Unstable/Runtime/Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ enum ExtractorRunResult
/// The extractor crashed.
/// </summary>
Error,
/// <summary>
/// The extractor was stopped with a clean shutdown.
/// But we need to restart it (possibly due to a revision change).
/// </summary>
RestartRequired
Copy link
Copy Markdown

@aasmundeldhuset aasmundeldhuset Apr 14, 2026

Choose a reason for hiding this comment

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

Nitpick that you don't have to address, but: adding a comma at the end will prevent an unnecessary modification of this line the next time an enum member is added.

}

/// <summary>
Expand Down Expand Up @@ -138,6 +143,7 @@ public async Task Run()
}
else if (result == ExtractorRunResult.CleanShutdown)
{
_activeLogger.LogInformation("Extractor stopped cleanly with policy {Policy}", _params.RestartPolicy);
// Shut down, if the extractor is configured to only restart on error.
if (_params.RestartPolicy != ExtractorRestartPolicy.Always)
{
Expand All @@ -147,6 +153,11 @@ public async Task Run()
// Otherwise, immediately restart.
backoff = 0;
}
else if (result == ExtractorRunResult.RestartRequired)
Copy link
Copy Markdown

@aasmundeldhuset aasmundeldhuset Apr 14, 2026

Choose a reason for hiding this comment

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

Not something that needs to be addressed in this PR, but: shouldn't this large if/else over an enum be a switch statement?

{
_activeLogger.LogInformation("Extractor stopped cleanly with restart required, restarting with backoff");
backoff = 1;
}

if (backoff == 0)
{
Expand Down Expand Up @@ -328,6 +339,7 @@ private async Task<ExtractorRunResult> BuildAndRunExtractor(ServiceProvider prov
{
using var internalTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_source.Token);
TExtractor extractor;
var shouldRestart = false;
Copy link
Copy Markdown

@aasmundeldhuset aasmundeldhuset Apr 14, 2026

Choose a reason for hiding this comment

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

This isn't used in the first try, so I think it should be declared above the second one, so that its limited scope is more clear.

try
{
if (_params.AddMetrics)
Expand Down Expand Up @@ -386,6 +398,7 @@ private async Task<ExtractorRunResult> BuildAndRunExtractor(ServiceProvider prov
{
_activeLogger.LogInformation("Revision changed, reloading config");
internalTokenSource.Cancel();
shouldRestart = true;
await extractorTask.ConfigureAwait(false);
}

Expand All @@ -394,6 +407,7 @@ private async Task<ExtractorRunResult> BuildAndRunExtractor(ServiceProvider prov
{
ExceptionDispatchInfo.Capture(extractorTask.Exception).Throw();
}

}
catch (OperationCanceledException) when (internalTokenSource.IsCancellationRequested)
{
Expand All @@ -415,7 +429,11 @@ private async Task<ExtractorRunResult> BuildAndRunExtractor(ServiceProvider prov

return ExtractorRunResult.Error;
}

if (shouldRestart)
Copy link
Copy Markdown

@aasmundeldhuset aasmundeldhuset Apr 14, 2026

Choose a reason for hiding this comment

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

Add a blank line above to visually differentiate this from the try/catch.

On a related note: this is a 100-line function that has got three quite distinct parts: the creation of the extractor, the waiting for the extractor or the "revision" change event, and the final restart check. I recommend splitting out the first two into separate functions - but that doesn't need to be done in this PR.

{
_activeLogger.LogInformation("Extractor stopped cleanly with policy {Policy}, restart is required", _params.RestartPolicy);
return ExtractorRunResult.RestartRequired;
}
return ExtractorRunResult.CleanShutdown;
}

Expand Down
Loading
⚡