fluentscheduler / FluentScheduler

Automated job scheduler with fluent interface for the .NET platform.
Other
2.68k stars 409 forks source link

Consider a non static API #148

Closed marcwittke closed 6 years ago

marcwittke commented 7 years ago

The static API makes it impossible to run integration tests in parallel. It is desirable to have multiple composition roots at the same time, having separate schedulers running.

tallesl commented 7 years ago

I don't like the static JobManager either.

I started to maintain this library back in 2015. Since then I made some new feature here and some renaming there, but I didn't fundamentally changed how the scheduling is done.

There's basically one scheduler for the whole thing. The scheduling work is performed by one timer, two collections, and three functions, all of them static. Having a single scheduler (one timer) uses few resources but isn't so great when it comes to flexibility and, as I learned, for maintainability.

I believe the library design would be much better off with each schedule having its own scheduler (its own timer). That makes things much more testable and predictable. There's the downside of having n timers running (costing more cpu/ram) instead of a single one, but I believe this tradeoff is worth it (but I didn't bechmark it yet).

I plan to redesign this scheduling (and get rid of the job factory as it is too, it's just too problematic [[1]] [[2]] [[3]] [[4]] [5]) in a new major version. I have a bare bones version of it in my machine, I'm waiting until it gets a little more mature to push into a new branch here.

It may take awhile, but it's coming!

pkmccaffrey commented 7 years ago

I plan to redesign this scheduling (and get rid of the job factory as it is too, it's just too problematic

How would DI for an IJob be accomplished then? Losing the ability to inject dependencies into a job would be a deal killer for me.

tallesl commented 7 years ago

Just capture the DI container via closure:

var myContainer;

JobManager.AddJob(
    () =>
    {
        var myResource = myContainer.Resolve<SomeResource>();
        // ...
    },
    s => s.ToRunEvery(5).Seconds()
);

Much cleaner than injecting it inside the library. Your DI container, your resource, your job.

pkmccaffrey commented 7 years ago

That works if you are adding a job that way, but what if you have a bunch of classes that implement IJob? I mean, I guess you could resolve all the dependencies and pass them into the IJob's constructor, but that doesn't seem very clean at all.

What's wrong with the JobFactory? It seems to work just fine.

tallesl commented 7 years ago

but what if you have a bunch of classes that implement IJob?

I'm considering removing the IJob interface entirely as well or, less extreme, just removing the <T> option (demanding you to instantiate it).

I mean, I guess you could resolve all the dependencies and pass them into the IJob's constructor, but that doesn't seem very clean at all.

If you're dealing with multiple dependencies on your job, why not creating a job type of your own and resolve all its dependencies at once?

var myContainer;

JobManager.AddJob(
    () => myContainer.Resolve<MyClass>().DoWhatever(), // look mom, no IJob!
    s => s.ToRunEvery(5).Seconds()
);

What's wrong with the JobFactory? It seems to work just fine.

It may "work" in the sense of hitting run and getting the desired result, but is it actually helpful?

pkmccaffrey commented 7 years ago

If you're dealing with multiple dependencies on your job, why not creating a job type of your own and resolve all its dependencies at once?

This is what I'm already doing. My jobs are constructor injected with multiple dependencies, which is why I'm using DI (via IJobFactory) to resolve them.

I suppose doing it the way you suggested (resolving the job from the DI container inside the AddJob call) would work -- all that changes is what/where is responsible for resolving the job type. It's a tiny bit more code to write to get a job up and running, but that's not really a big deal.

tallesl commented 7 years ago

Just passing by to say that, while it's still far from done, it's coming along on fluentscheduler/redesign.

PonchoPowers commented 6 years ago

For the code you have why not commit it to a new branch so people can work with you on it, that way the community as a whole can shape it too while we have a chance to make a bigger influence.

MNF commented 6 years ago

It’s available as redesign branch https://github.com/fluentscheduler/FluentScheduler/tree/redesign

tallesl commented 6 years ago

See #214.

atanukbiswas79 commented 5 years ago

is there a way to get a status of a specific job? such as Running, Stopped or ready next schedule etc.

tallesl commented 5 years ago

Right now you can't.

(Why asking on this issue though?)