Skip to content
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.

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

Allow "one-time" jobs #43

Closed
linkdotnet opened this issue May 6, 2024 · 7 comments · Fixed by #70
Closed

Allow "one-time" jobs #43

linkdotnet opened this issue May 6, 2024 · 7 comments · Fixed by #70
Assignees
Labels
help wanted Extra attention is needed

Comments

@linkdotnet
Copy link
Member

linkdotnet commented May 6, 2024

Allow the execution of jobs that are only run once at the beginning of the lifecycle. This could be some migration work or other tasks that should run very early on in the lifecycle.

Example

Services.AddNCronJob(options => 
{
  options.AddJob<MyJob>().RunOnce();
});

This will lead to a direct execution. RunOnce can have two overloads:

public sealed class JobOptionBuilder
{
+   public ParameterBuilder RunOnce();
+   public ParameterBuilder RunOnce(TimeSpan delay);
+   public ParameterBuilder RunOnce(DateTimeOffset absoluteDay);
}

To make that work, the CronRegistry has to be extended to distinguish between one-time jobs and CRON jobs.

internal sealed partial class CronRegistry 
{
+   public IReadOnlyCollection<RegistryEntry> GetAllOneTimeJobs() => oneTimeJobs;
}

This then can be picked up by the CronSchedulder to execute those once.

@linkdotnet linkdotnet added help wanted Extra attention is needed good first issue Good for newcomers labels May 6, 2024
@falvarez1
Copy link
Member

This is a good idea and enhancement! I see the value in supporting tasks that run once at the beginning of the lifecycle, such as migrations or data cleanup. However, these tasks are somewhat outside the primary purpose of the CronScheduler object, which is designed to handle recurring tasks based on Cron expressions.

However, your suggestion brings up an interesting point about the flexibility of job scheduling. I agree that it could be beneficial to have a more generalized scheduling framework. This would allow for different types of scheduling mechanisms: Cron, One-Time, Periodic/Interval, and potentially others like iCalendar/RRule, although iCalendar might be beyond the current scope of this library.

To incorporate this flexibility, one approach could be to develop a common abstraction or interface, say IScheduler, which could define the basic functionality expected from any scheduler type. Then, specific classes like CronScheduler, OnceScheduler and IntervalScheduler could implement this interface, each handling their specific types of scheduling. We would also need to separate out the trigger metadata from the RegistryEntry. Possibly through an ITrigger interface that outlines common requirements for triggers.

This design would keep the library modular and maintain SoC.

@linkdotnet
Copy link
Member Author

However, these tasks are somewhat outside the primary purpose of the CronScheduler object, which is designed to handle recurring tasks based on Cron expressions.

Yes you are right - and while it might be easy to push that functionality inside CronScheduler, it might not be beneficial in the long term.
That said. With #25, we can have a better data model. I tried that yesterday in a small PoC, but the issue is observability (we can run fewer tests because it behaves further like a black box).

@linkdotnet linkdotnet removed the good first issue Good for newcomers label May 8, 2024
@linkdotnet
Copy link
Member Author

Basically, with our abstraction, namely QueueWorker, this shouldn't be a big problem anymore.

While the main theme of the library is (and will be) CRON, I don't see much speaking against introducing this concept.
Internally we could do "literally" the same as we do with instant jobs.

@falvarez1
Copy link
Member

Yes it's very similar. If we want to collect these and run before any other job type however, we can add a property to JobDefinition that identifies this type of startup job. Then run something like this right before the call the ScheduleInitialJobs.

private async Task ProcessStartupJobs(CancellationToken stopToken)
{
    var startupJobs = registry.GetAllCronJobs().Where(j => j.IsStartupJob).ToList(); // or registry.GetAllOneTimeJobs
    if (startupJobs.Any())
    {
        var startupTasks = new List<Task>();

        foreach (var job in startupJobs)
        {
            startupTasks.Add(CreateExecutionTask(job, stopToken));
        }

        await Task.WhenAll(startupTasks);
    }
}

I don't think there's a need to juggle with signals to try to embed these into the regular loop.

@falvarez1
Copy link
Member

@linkdotnet for clarification, jobs we want to run at startup, like migrations, implies that they need to start and complete before anything else. The signature you designed assumes both a one time job that can run at startup, and a one time job that can run at any time.
Is it safe to say that options.AddJob<MyJob>().RunOnce(); is a startup job, and options.AddJob<MyJob>().RunOnce(TimeSpan.FromMinutes(5)); behaves more like an instant job that is scheduled?

If so then I think the method name would be clearer as RunAtStartup() instead of RunOnce().

@falvarez1 falvarez1 self-assigned this May 22, 2024
@linkdotnet
Copy link
Member Author

linkdotnet commented May 22, 2024

That is an interesting thought. I did think about them more simplistic.
Basically there shouldn't be a difference between:

builder.Services.AddNCronJob(b => b.AddJob<TJob>().RunOnce());

Versus:

builder.Services.AddNCronJob(b => b.AddJob<TJob>());
// ...
app.Services.GetRequiredService<IInstantJobRegistry>().RunInstantJob<TJob>();

In both cases I would expect that
a) Retry-Policy
b) Concurrency-Control
c) Global Job-limit

will apply. After all - there are "still jobs".

I do think it would make sense to refine the requirement in the sense that we only allow RunAtStartup and drop support for the delayed execution. Mainly because I don't see much need (I proposed it to align the API with what we already have) and the user has a workaround to accomplish that already.

@linkdotnet
Copy link
Member Author

Another question you raised (or answered) in your PR is if a "Startup-Job" should be awaited before any other "regular" CRON job or instant job gets executed.
Obviously we could just introduce a bool flag for that - but that seems not like the right path.

falvarez1 added a commit that referenced this issue May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants