search

NCronJob-Dev / NCronJob

A Job Scheduler sitting on top of IHostedService in dotnet.
https://docs.ncronjob.dev/
MIT License
158 stars 11 forks source link

Deal with duplicates in a uniform manner #128

Closed nulltoken closed 2 weeks ago

nulltoken commented 2 weeks ago

Doesn't cringe:

        ServiceCollection.AddNCronJob(n => n
            .AddJob<SimpleJob>(p => p.WithCronExpression("* * * * *"))
            .AddJob<SimpleJob>(p => p.WithCronExpression("* * * * *")));

vs

Fails with InvalidOperationException

        ServiceCollection.AddNCronJob(n => n
            .AddJob<SimpleJob>(p => p.WithCronExpression("* * * * *").WithName("Name"))
            .AddJob<SimpleJob>(p => p.WithCronExpression("* * * * *").WithName("Name")));

The code does a lot to avoid duplicated jobDefinitions.

However, the API is a tad inconsistent here with regards to its behavior.

I'd be in favor of always failing.

linkdotnet commented 2 weeks ago

Hmm good catch. Would be nice if we can even do this on v3.

linkdotnet commented 2 weeks ago

I can remember we had some discussion month ago about this (https://github.com/NCronJob-Dev/NCronJob/discussions/79). One possible solution was just to "ignore" the fact and use something like a ISet to automatically remove duplicates.

So the question is whether or not this is an "exceptional" case in which the system can't behave properly anymore. So either we throw or just drop the duplicated entry.

linkdotnet commented 2 weeks ago

I would still argue: Let's drop duplicates, create a warning and let the user decide what's going on. JobNames are different. Independent of the type, they have to be unique! So even if you do:

ServiceCollection.AddNCronJob(n => n
            .AddJob<SimpleJob>(p => p.WithCronExpression("* * * * *").WithName("Name"))
            .AddJob<OtherUnrelatedJob>(p => p.WithCronExpression("1 1 * * *").WithName("Name")));

That has to fail.The system can not operate and recover if we encounter this situation.

linkdotnet commented 2 weeks ago

Sorry for the spam. I just checked, we are already dropping duplicates. So the only thing would be to at least print a warning that we detected a duplicate.

nulltoken commented 2 weeks ago

I can remember we had some discussion month ago about this (https://github.com/NCronJob-Dev/NCronJob/discussions/79)

Thanks for the pointer.

With regards to the following, those are clearly two different job definitions.

b.WithCronExpression("0 * * * *").WithParameter(100)
 .And
 .WithCronExpression("0 * * * *").WithParameter(200)

Whereas I can't find a proper use case to allow

b.WithCronExpression("0 * * * *")
 .And
 .WithCronExpression("0 * * * *")

As no one has complained about it, I'd say, keep v3 as is from a behavioral standpoint (and log a warning) and throw in v4.

nulltoken commented 2 weeks ago

And I do agree that we need names being unique, as those names will certainly play a role in the disambiguation all the weird registration corner cases from #108.

linkdotnet commented 2 weeks ago

When I started that library - I had a very very small use case for it. Very interesting where we got here :D

In any case - for sure I will log a warning for v3 and make myself some thoughts about throwing.

nulltoken commented 2 weeks ago

Very interesting where we got here :D

:D

This lib fills a need without going full blown Enterprise-y. It brings a real benefit without going in your way.

nulltoken commented 2 weeks ago

In any case - for sure I will log a warning for v3 and make myself some thoughts about throwing.

With regards to throwing for v4, below my two cents:

There are two use cases for registering new jobs:

How about:

linkdotnet commented 2 weeks ago

In any case - for sure I will log a warning for v3 and make myself some thoughts about throwing.

With regards to throwing for v4, below my two cents:

There are two use cases for registering new jobs:

  • In Program.cs:

    • This is most certainly a misconfiguration. In that context, I see throwing as a safe bet. The app hasn't started yet. The tests will crash. During a standard CI/CD deployment, there are plenty of time to catch it and fix it. Most apps nowadays leverage Blue/Green deployment practice through AppService staging slot (similar offerings exists for both AWS, Google Cloud, ...). Even if a dedicated swapping mechanism exists, as the app doesn't start, readiness probes won't allow it to be enlisted as a healthy node in a webfarm.
    • If we only only logs warnings, they won't pop up during the tests execution (unless a very specific xunit/ILogger configuration is set up during the test execution). They will only be displayed when someone starts the app and glance at the traces. The only safety net in that context is the code peer review. And it might be easy to miss.

  • Through the RuntimeRegistry

    • Depending on how thorough the automated (and/or manual) testing is, the team ability to easily recover from a crash (DORA MTTR), the number of environments an app goes through before reaching Production, ... some teams will indeed prefer to avoid having exceptions at runtime and will be happy with warnings. Others will prefer a crash to happen (because, once again, that's most certainly a sign of misconfiguration, or a design issue).

How about:

  • Always failing in Program.cs
  • Offering through UseNCronJobs(NCronJobOptions) a way for the user to decide how the lib should behave when detecting the creation of duplicated jobdefinitions through the RuntimeRegistry?

Hmm good question. We could also offer a TryRegister or a configuration on the runtime registry. The problem is that a user can add multiple jobs via the registry in one go. Maybe we should not allow this in the first place. Otherwise it is hard to tell the user which job did and which didn’t get added

nulltoken commented 2 weeks ago

Hmm good question. We could also offer a TryRegister or a configuration on the runtime registry.

Going further, maybe a TryRegister(xxxx, [NotNullWhen(false)] out Exception exception)?

Then the user would be free to either log the exception, feed it to an Obervability container or just throw it...

linkdotnet commented 2 weeks ago

Hmm good question. We could also offer a TryRegister or a configuration on the runtime registry.

Going further, maybe a TryRegister(xxxx, [NotNullWhen(false)] out Exception exception)?

Then the user would be free to either log the exception, feed it to an Obervability container or just throw it...

For the time being this seems to be the most straightforward way. In any case, I will disallow registering multiple jobs at the same time. Imagine a user registers 5 jobs, where the last one is problematic. We would need some kind of "transaction" to ensure, that all or none are saved.

nulltoken commented 2 weeks ago

Imagine a user registers 5 jobs, where the last one is problematic. We would need some kind of "transaction" to ensure, that all or none are saved.

I do agree!

linkdotnet commented 2 weeks ago

Done in #129