[WIP] Adventures in the amazing world of specification of dependencies and predictiveness

[nulltoken]

Pull request description

@linkdotnet Early work in the land of #108

Not a real PR as of now, rather a test bed for expectations (and tests that we'd like to see passing).

PR meta checklist

• Pull request is targeted at main branch for code
• Pull request is linked to all related issues, if any.

Code PR specific checklist

• My code follows the code style of this project and AspNetCore coding guidelines.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• I have updated the appropriate sub section in the CHANGELOG.md.
• I have added, updated or removed tests to according to my changes.
  • All tests passed.

[nulltoken]

@linkdotnet Not all tests are passing.

Before going further with how the code should be changed to make them pass, I'd like some feedback from you about the following points:

• Do the new added tests make sense?
• What are new scenarios that we'd like to add?

Note: I haven't wandered in the region of dynamic jobs. Yet.
Before diving into that, I'd like to ensure "how the lib should behave with 'simpler' jobs" is defined.

[linkdotnet]

I will have a look at this at the end of the week - unfortunately I am a bit swamped with stuff.

[linkdotnet]

I dont think we should throw here in the first place. That is an absolut valid case and the instant job shouldn't be linked to "cron" jobs and their configuration in any way.

We also don't "assume" the parameter for an instant job based on the configuration inside the container. The user explicitly has to pass in the value he or she wants.

I know that doesn't make it easier but maybe shows that JobDefinition is really really a bad container at the moment.

Basically, we have Jobs, Runs, and somewhat a trigger. JobDefinition tries to bundle all those 3 things somewhat together. At least the "Jobs" and "Trigger" part and spawns the "run".

[nulltoken]

Updated the expectation:

        ServiceCollection.AddNCronJob(n =>
        {
            n.AddJob<ParameterJob>(s => s.WithCronExpression("* * 30 2 *"));
            n.AddJob<ParameterJob>(s => s.WithCronExpression("* * 31 2 *"));
        });

should no longer be expected to throw.

However, I've added

        ServiceCollection.AddNCronJob(n =>
        {
            n.AddJob<ParameterJob>(s => s.WithCronExpression("* * 30 2 *").WithParameter("13"));
            n.AddJob<ParameterJob>(s => s.WithCronExpression("* * 31 2 *"));
        });

which I believe should throw (as the behavior of the job may be altered by the parameter)

[linkdotnet]

I see where you are driving at. Imagine the following registration:

builder.Services.AddNCronJob(o => 
{
  // Here comes your new function into play
  o.AddJob<...>(w => w.WithCronExpression("* * * * *")).ExecuteWhen(s => s.RunJob("Name", null));

  o.AddJob<DependentJob>(o => o.WithCronExpression("* * * * *").WithName("Name"));
});

Technically the user registered a job with the given job name. The order inside a container registration shouldn't be important.

And that is where your useNCronJobAsync proposal from a few days may come in handy.

[linkdotnet]

But this is more of an exotic use case (at least in my mind).

[nulltoken]

But this is more of an exotic use case (at least in my mind).

I'm not sure. I've tried a spike where all dependencies were required to be registered before jobs leveraging them.
Although that works, that's an awful user experience.

The order inside a container registration shouldn't be important.

I have an idea how to resolve this

[linkdotnet]

We might want to special case that part. If the job is typeof(DynamicJobFactory) aka Lambda expression, then this will be more confusing than helpful

[nulltoken]

I know. I'm starting with the easy part. I'll tackle Dynamic jobs once we've straighten the general expectations on typed jobs. (through expected passing/failing tests)

[linkdotnet]

Hmmm I am not sure why this should throw.

In my mental model CRON jobs and instant jobs are two different things. The only thing they share is the execution logic but none of its metadata (well technically some of it like the retrypolicy stuff, dependent jobs and whether or not they can be executed, but nothing related to CRON jobs and its parameter).
Furthermore, users have the ability to add CRON jobs (IRuntimeRegistry.TryRegister) and parameters (IRuntimeRegistry.UpdateParameter) or update schedules.

That might lead to a situation where

1. The initial Instant job A works because there was no parameter specified
2. IRuntimeRegistry.UpdateParameteris called for A with a new parameter
3. Now the instant job fails with the given exception

The user would need to keep track of the parameters and needs to set it temporarily to something else or null so the instant trigger succeeds.

More and more we might want to check of we need distinct objects like JobDefinition, JobRun and Trigger.

That would solve some of our issues, mainly:

1. Dependent jobs can hang either on a Trigger or a JobDefinition (so they get triggered for instant jobs as well). The location where you call ExecuteWhen is key. But takes out ambiguity. Maybe we can even have distinct methods for that so it is painfully clear: That should be straightforward as can provide overloads to AddJob. For example if you "only" call: AddJob().ExecuteWhen it should always be on the type aka Jobdefinition, but: AddJob(c => c.WithCronExpression()) should sit on the trigger.
2. Would make it also easier to distinct if we have duplicates. The JobDefinition should always be unique and we just have a bunch of triggers.

Sorry for the wall of text, but seeing that our system is so ambiguous tells me, we need to go in a different direction. Extending the current system may lead to more confusion

[linkdotnet]

A trigger might have a definition like that:

/// <summary>
/// Base interface for job triggers
/// </summary>
internal interface IJobTrigger 
{
    JobDefinition JobDefinition { get; }
    object? Parameter { get; }
    JobPriority Priority { get; }
    DateTimeOffset? GetNextExecutionTime(TimeProvider timeProvider);
}

/// <summary>
/// Trigger for CRON-based scheduling
/// </summary>
internal sealed class CronTrigger : IJobTrigger
{
    private readonly CronExpression cronExpression;
    private readonly TimeZoneInfo timeZone;

    public CronTrigger(
        JobDefinition jobDefinition,
        string cronExpression,
        TimeZoneInfo? timeZone = null,
        object? parameter = null)
    {
        JobDefinition = jobDefinition;
        this.cronExpression = CronExpression.Parse(cronExpression);
        this.timeZone = timeZone ?? TimeZoneInfo.Utc;
        Parameter = parameter;
        Priority = JobPriority.Normal;
    }

    public JobDefinition JobDefinition { get; }
    public object? Parameter { get; }
    public JobPriority Priority { get; init; }

    public DateTimeOffset? GetNextExecutionTime(TimeProvider timeProvider)
    {
        var utcNow = timeProvider.GetUtcNow();
        return cronExpression.GetNextOccurrence(utcNow, timeZone);
    }
}

/// <summary>
/// Trigger for immediate or scheduled one-time execution
/// </summary>
internal sealed class InstantTrigger : IJobTrigger 
{
    public InstantTrigger(
        JobDefinition jobDefinition,
        object? parameter = null,
        DateTimeOffset? scheduledTime = null)
    {
        JobDefinition = jobDefinition;
        Parameter = parameter;
        ScheduledTime = scheduledTime;
        Priority = JobPriority.High;
    }

    public JobDefinition JobDefinition { get; }
    public object? Parameter { get; }
    public JobPriority Priority { get; init; }
    public DateTimeOffset? ScheduledTime { get; }

    public DateTimeOffset? GetNextExecutionTime(TimeProvider timeProvider) => 
        ScheduledTime ?? timeProvider.GetUtcNow();
}

[linkdotnet]

Also we have this beauty which might get solved:

builder.Services.AddNCronJob(o => 
{
  o.AddJob<MyJob>().ExecuteWhen(s => s.RunJob<JobA>());
  o.AddJob<MyJob>(s => s.WithCronExpression("* * * * *")).ExecuteWhen(s => s.RunJob<JobB>());

If "* * * * *" is true - what dependent jobs should be executed? I'd argue JobA **and JobB. And this would easy to model with the Trigger. well "easier".

[nulltoken]

In my mental model CRON jobs and instant jobs are two different things. The only thing they share is the execution logic but none of its metadata (well technically some of it like the retrypolicy stuff, dependent jobs and whether or not they can be executed, but nothing related to CRON jobs and its parameter).

@linkdotnet Hrmpf. I completely overlooked this paragraph. And some of it actually triggers a lot of questions in my mind.

I've never linked CRON expression with parameters. In my mind, those two things are completely orthogonal (FWIW,
I've got a local branch with some code change slightly tweaking the fluent interface in order to allow a WithParameter().WithName() chain without a WithCronExpression() root.)

Reasoning (at least how I see it):

• Parameters are a way to optionally alter the behavior of how a job runs.
• Those parameters can be set through a AddJob() call
• They can be overridden through an instant job execution

Thoughts? Is there a world where parameters don't depend on Cron expressions?

[nulltoken]

Going a little down further this path, I "see" actually two different phases/steps (independently of how the API make them happen).

The Definition:

• The root job, its parametrization, the orchestration of dependent steps (with RunJob() allowing the optional override of the parameter). Those steps being either inline jobs or "references" to other root jobs.

The Scheduling:

• A way to trigger root jobs at regular intervals

All layers of new registrations contributing to the Definition should lead to a list of entries and references that are unambiguous. When an ambiguity is detected, the lib complains and the "Definition" should be updated to leverage "Names" to resolve those ambiguities (RunJob() may have to be updated to accept a jobName to make this happen and JobOptionBuilder might also need to expose WithName()).

Instant Jobs are only a special case of Scheduling

RuntimeRegistry ensures through the TryRegister() method that the Definition is kept unambiguous.

To sum up:

• Easy job setups would still be easy to define.
• Complex orchestrations involving reused jobs in different execution contexts would be possible in a safe way, the registration engine ensuring the absence of ambiguity at runtime

What's your feeling about this?

[linkdotnet]

I've never linked CRON expression with parameters. In my mind, those two things are completely orthogonal (FWIW,

So right now that is the model. Defining a job: AddJob<MyJob>(s => s.WithCronExpression("* * * * *").WithParameter("foo")) is exactly that.

I've got a local branch with some code change slightly tweaking the fluent interface in order to allow a WithParameter().WithName()

True that - it is currently only possible to register CRON jobs with a name. If we take the current world only, then WithParameter without a CRON expression doesn't make sense, as you can specify parameters for instant job when calling them.

They can be overridden through an instant job execution

That is an fair assumption - and I like that mental model. The current API might make it a bit awkward. Imagine you register a Job with two CRON Expressions and each of them has different parameters. Now you trigger an instant job without passing in arguments - what will be used?

Going a little down further this path, I "see" actually two different phases/steps

It goes exactly into the direction you mentioned some time ago, where AddNCronJob is the collection phase and Use is the configuration/kickstart phase.

[nulltoken]

That is an fair assumption - and I like that mental model. The current API might make it a bit awkward. Imagine you register a Job with two CRON Expressions and each of them has different parameters. Now you trigger an instant job without passing in arguments - what will be used?

This is actually where an ambiguity would be raised at runtime. In order to resolve it, one would have to identify a specific job through its name to trigger it.

Some flavors of ambiguities can be detected before runtime (during the registration) and would prevent the app from running.

Some others (as the one you just described) can be identified as well (and for instance logged as a warning (eg. "Job X is registered Y times without name. Trying to invoke it through an InstantJob would lead to a runtime exception.") but wouldn't prevent the app from starting as the lib cannot be 100% sure whether or not that InstantJob invocation will happen or not).

True that - it is currently only possible to register CRON jobs with a name. If we take the current world only, then WithParameter without a CRON expression doesn't make sense, as you can specify parameters for instant job when calling them.

There's a case when this could make sense even in the current world.

n.AddJob<Reporting>(s => s..WithParameter("using standard channels"));
n.AddJob<TaskOne>(s => s.WithCronExpression(Monthly)))
    .ExecuteWhen(s => s.RunJob<Reporting>());
n.AddJob<TaskTwo>(s => s.WithCronExpression(Daily)))
    .ExecuteWhen(s => s.RunJob<Reporting>());
n.AddJob<TaskThree>(s => s.WithCronExpression(Daily)))
    .ExecuteWhen(s => s.RunJob<Reporting>());

And then from an InstantJob .ForceRunInstantJob<TaskOne>("using emergency channels");

[linkdotnet]

By any chance, do you know how Quartz/Hangfire handles such situations?

This is actually where an ambiguity would be raised at runtime. In order to resolve it, one would have to identify a specific job through its name to trigger it.

I fear that if we do this, we declassify Instant jobs to the extent that you always have to take care of naming, just because you want to trigger a job with different names. So you have to register the job twice at least - once for instant jobs, once for CRON (which could be also a route - offering a AddInstantJob method that operates on its own term).

The funny part is that I use this behavior in my own projects with NCronJob. I do have a Cron Job with Parameter "A" but the instant job triggers with Parameter "B" and I don't wanna think too much about this while building up the container.

[codecov]

Codecov Report

Attention: Patch coverage is 69.56522% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...CronJob/Configuration/Builder/DependencyBuilder.cs	41.66%	5 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
src/NCronJob/Registry/JobRegistry.cs	96.15% <100.00%> (+0.27%)	⬆️
...CronJob/Configuration/Builder/DependencyBuilder.cs	65.00% <41.66%> (-35.00%)	⬇️