NCronJob icon indicating copy to clipboard operation
NCronJob copied to clipboard

Allow "one-time" jobs

Open linkdotnet opened this issue 1 year ago • 7 comments
trafficstars

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 avatar May 06 '24 08:05 linkdotnet

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.

falvarez1 avatar May 08 '24 03:05 falvarez1

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 avatar May 08 '24 06:05 linkdotnet

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.

linkdotnet avatar May 22 '24 11:05 linkdotnet

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 avatar May 22 '24 16:05 falvarez1

@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 avatar May 22 '24 17:05 falvarez1

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 avatar May 22 '24 19:05 linkdotnet

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.

linkdotnet avatar May 22 '24 19:05 linkdotnet