Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.
/ NuGet.Jobs Public archive

[Package Signing] Make the Revalidate Certificate job initialize once per day #405

Merged
merged 6 commits into from
Apr 16, 2018

Conversation

loic-sharma
Copy link
Contributor

@loic-sharma loic-sharma commented Apr 16, 2018

The Job infrastructure used to call the job's Init method on each invocation of the job loop, which caused the Revalidate job to load secrets repeatedly. This change adds the option to have a minimum wait time in between a job's Init calls.

The Job infrastructure calls the job's Init method on each invocation of
the job loop. This caused the Revalidate job to load secrets repeatedly.
Copy link

@scottbommarito scottbommarito left a comment

Choose a reason for hiding this comment

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

To be honest, this seems like a hack around the job runner. With this change, the job is responsible for looping its code over and over again until it determines it has been running for too long and needs to initialize, which should be a responsibility of the job runner. Additionally, preventing config from being reloaded too frequently sounds like something that all jobs could benefit from.

I think a better change would be to add some sort of InitMinimumFrequencySeconds config that is used by JsonConfigurationJob or JobRunner itself to determine whether or not it needs to re-initialize.

@loic-sharma loic-sharma changed the title [Package Signing] Make the Revalidate Certificate job initialize just once [Package Signing] Make the Revalidate Certificate job initialize once per day Apr 16, 2018
@loic-sharma
Copy link
Contributor Author

@scottbommarito I added ReinitializeAfterSeconds CLI argument that JobRunner uses to determine whether or not it should re-initialize.

@loic-sharma loic-sharma force-pushed the loshar-revalidate-startup branch from 7ffc46d to 5e75c48 Compare April 16, 2018 22:20

private static bool ShouldInitialize(int? reinitializeAfterSeconds, Stopwatch timeSinceInitialization)
{
// If there is no wait time between reinitializations, always reinitialize.

Choose a reason for hiding this comment

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

nit: you can make this one big boolean return!

(note: I changed the comment on the second condition)

return
    // If there is no wait time between reinitializations, always reinitialize.
    !reinitializeAfterSeconds.HasValue ||

    // A null time since last initialization indicates that the job hasn't been initialized yet.
    timeSinceInitialization == null ||

    // Otherwise, only reinitialize if the reinitialization threshold has been reached.
    (timeSinceInitialization.Elapsed.TotalSeconds > reinitializeAfterSeconds.Value);

Copy link
Contributor Author

@loic-sharma loic-sharma Apr 16, 2018

Choose a reason for hiding this comment

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

I considered that as well, but I think that it's a little easier to grok as it is.

Copy link
Contributor Author

@loic-sharma loic-sharma Apr 16, 2018

Choose a reason for hiding this comment

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

Updated the comment with the comment you suggested.

@loic-sharma loic-sharma force-pushed the loshar-revalidate-startup branch from 57ded38 to 03622b0 Compare April 16, 2018 23:05
@loic-sharma loic-sharma force-pushed the loshar-revalidate-startup branch from 1bedb7b to c4434fc Compare April 16, 2018 23:07
@loic-sharma loic-sharma merged commit 84c6b36 into dev Apr 16, 2018
@loic-sharma loic-sharma deleted the loshar-revalidate-startup branch April 16, 2018 23:29
joelverhagen pushed a commit that referenced this pull request Oct 26, 2020
… per day (#405)

The Job infrastructure used to call the job's `Init` method on each invocation of the job loop, which caused the Revalidate job to load secrets repeatedly. This change adds the option to have a minimum wait time in between a job's `Init` calls.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants