-
Notifications
You must be signed in to change notification settings - Fork 21
[Revalidation] Pause if the ingestion pipeline is unhealthy #504
Conversation
@@ -115,7 +115,7 @@ | |||
<Version>2.27.0</Version> | |||
</PackageReference> | |||
<PackageReference Include="NuGetGallery.Core"> | |||
<Version>4.4.5-dev-35743</Version> | |||
<Version>4.4.5-loshar-health-36421</Version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this before merging this change.
"Could not find component path {ComponentPath} in status health blob!", | ||
_config.ComponentPath); | ||
|
||
throw new InvalidOperationException($"Could not find component path '{_config.ComponentPath}' in status health blob!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if this exception is thrown, how will the caller treat it? Can we just return false
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this exception is thrown, it'll be caught and reported by the JobRunner
. The job would then exit and restart.
Returning false
if the path can't be found sounds fine to me too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
.Setup(s => s.GetFileAsync(_config.ContainerName, _config.StatusBlobName)) | ||
.ThrowsAsync(new Exception()); | ||
|
||
await Assert.ThrowsAsync<Exception>(() => _target.IsHealthyAsync()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit Assert.Same
on the exception instance to verify no wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
private readonly HealthConfiguration _config; | ||
private readonly HealthService _target; | ||
|
||
public HealthServiceFacts() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test th missing component path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm misunderstanding you, this should be covered by ThrowsIfStatusBlobIsMissingExpectedPath
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the test to AssumesUnhealthyIfComponentCannotBeFoundInStatusBlob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address comments then
0ae7826
to
af98945
Compare
Verified this on DEV with a manually created incident. |
…es per ID (#504) Progress on NuGet/NuGetGallery#6475
This integrates with @scottbommarito's status work to pause the job if the ingestion pipeline is unhealthy. Part of https://github.com/NuGet/Engineering/issues/1128
…es per ID (#504) Progress on NuGet/NuGetGallery#6475
This integrates with @scottbommarito's status work to pause the job if the ingestion pipeline is unhealthy.
Part of https://github.com/NuGet/Engineering/issues/1128