[Revalidation] Request revalidation #477
Conversation
loic-sharma
requested review from
agr,
shishirx34,
zivkan,
ryuyu,
dtivel,
chenriksson,
cristinamanum,
skofman1 and
scottbommarito
| /// Check whether the killswitch has been activated. If it has, all revalidation operations should be halted. | ||
| /// </summary> | ||
| /// <returns>Whether the killswitch has been activated.</returns> | ||
| Task<bool> IsKillswitchActiveAsync(); |
There was a problem hiding this comment.
Shouldn't it be CancellationToken that you'd pass around (a property in this exact interface?) instead of a function returning bool that has to be called every once in a while? We'd get cancellation of library functions essentially for free then...
There was a problem hiding this comment.
The killswitch value comes from an Azure blob that will be polled, hence this API design.
I need to update this PR, but, the killswitch will be implemented using this: NuGet/ServerCommon#169
|
|
||
| namespace NuGet.Services.Revalidate | ||
| { | ||
| public interface ITelemetryService | ||
| public interface ISingletonService |
| /// Delay the current task to achieve the desired revalidation rate. | ||
| /// </summary> | ||
| /// <returns>Delay the task to ensure the desired revalidation rate.</returns> | ||
| Task OnRevalidationEnqueuedAsync(); |
There was a problem hiding this comment.
Method that produces a task that delays should probably have Delay in the name :)
Same for the method below.
There was a problem hiding this comment.
I like that. Fixed
|
|
||
| var next = await _validationContext.PackageRevalidations | ||
| .Where(r => r.Enqueued == null) | ||
| .Where(r => r.Completed == false) |
There was a problem hiding this comment.
Please don't hate me but I like how those two Wheres look next to each other. Rules are mean to be broken! 🙈
| .Where(p => p.NormalizedVersion == revalidation.PackageNormalizedVersion) | ||
| .FirstOrDefaultAsync(); | ||
|
|
||
| return (package == null || package.PackageStatusKey == PackageStatus.Deleted); |
There was a problem hiding this comment.
This code retrieves complete Package object from DB, checks its single property, then discards it. Consider adding something like .Select((PackageStatus?)r.PackageStatus) (not sure what's exact type here) before calling FirstOrDefault.
There was a problem hiding this comment.
Good catch. Fixed
| return; | ||
| } | ||
| } | ||
| while (runTime.Elapsed <= _config.ShutdownWaitInterval); |
There was a problem hiding this comment.
Some crazy idea: instead of creating a stopwatch to track the execution time, we could start a var t = Task.Delay(_config.ShutdownWaitInterval), then just check t.IsCompleted as the while condition. Never tried myself.
There was a problem hiding this comment.
Mm, I think I prefer the Stopwatch pattern. Stopwatches are known to be very accurate, whereas I'm not sure how accurate Task.Delay + IsCompleted would be.
| namespace NuGet.Services.Revalidate | ||
| { | ||
| public interface IHealthService | ||
| { |
There was a problem hiding this comment.
Is this interface meant to be used by all of our jobs?
There was a problem hiding this comment.
This interface is specific to the revalidate job only. It will only check the health of services that may be affected by revalidations (so it won't check the gallery or the statistics pipeline)
| { | ||
| using (_telemetryService.TrackDurationToStartNextRevalidation()) | ||
| { | ||
| // Check whether a revalidation can be started. |
There was a problem hiding this comment.
Maybe add comments for the conditions when a revalidation cannot be started.
There was a problem hiding this comment.
Added. Let me know if you think the comment can be improved.
Adds the core logic of the revalidation job so that it wakes up, determines the next package to revalidate, and enqueues it to the Orchestrator. Addresses https://github.com/NuGet/Engineering/issues/1443
Adds the core logic of the revalidation job so that it wakes up, determines the next package to revalidate, and enqueues it to the Orchestrator. This change does not cover:
Addresses https://github.com/NuGet/Engineering/issues/1443