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

Redesign branch #214

Closed
tallesl opened this issue Sep 2, 2018 · 40 comments
Closed

Redesign branch #214

tallesl opened this issue Sep 2, 2018 · 40 comments

Comments

@tallesl
Copy link
Contributor

tallesl commented Sep 2, 2018

There's a fundamental issue on the current version of the library: everything is done by one scheduler. The scheduling work is performed by one timer, two collections, and three functions, all of them static. Having a single scheduler (one timer) uses fewer resources but isn't so great when it comes to flexibility and, as I learned, for maintainability.

The library will be better off with each schedule having its own scheduler (its own timer). That makes things more testable and predictable. There's an ongoing library redesign that does precisely that.

I'm freezing the current version (no new features). I kindly ask you to be patient and wait for the redesign to be done before asking for/implementing new things.

A heads up on what is about to change:

IJob is gone

No interfaces to implement, just good old Action.

JobManager is gone

You'll be instantiating your own schedules, and the scheduling is performed by the schedule itself. No more static manager.

JobRegistry is gone

Since there's no manager, there's no need to build a registry to initialize it. However, now you'll be able to group schedules and manage them all at once (start, stop, reset, listen for events).

JobFactory is gone

I'm finally getting rid of the whole dependency injection thing, which is a source of a lot of stupid issues. Since now you'll be the one instantiating and owning the schedules, there's no "but the library is a composition root" excuse anymore.

And if you need to resolve some instance inside your job, just get it via a closure.

Cron is coming

Thanks to NCrontab, support for cron expressions is finally coming.

@MNF
Copy link

MNF commented Sep 2, 2018

So many things gone, so it sounds scary.
Do you have forward- compatible syntax, I.e how to use the current library , that the code will work in a new version?
Also please consider instead of complete removal of classes(at least most oftenly used) mark them as obsolete and support old syntax during transition period until the next major version.

The redesign branch has 200 commits behind master. You need to merge them to clarify to your users, that you creating a next version, not writing a new library from the scratch.

@tallesl
Copy link
Contributor Author

tallesl commented Sep 2, 2018

If you are afraid of some particular design decision please bring it to the table and I would love to discuss it. Now, if you are afraid just for being something new/different, please don't be.

Version 5 will stay as it is without any further API change (just eventual needed bug fixes). Version 6, future version being developed on the redesign branch, will break compatibility completely with version 5.

It's not that the branch has X commits behind master, it doesn't have any commit in common with master, it was made from scratch.

@tallesl tallesl closed this as completed Sep 2, 2018
@tallesl tallesl reopened this Sep 2, 2018
@MarkusKgit
Copy link

Hi @tallesl,
first of all thanks for the ongoing work on this library. One question for the redesign:
Now that there will be individual schedulers - will there be an option to provide your own scheduler? I love how Reactive Extensions have tackled this, especially with their TestScheduler (see here) It makes testing with considering time a lot easier.

@MNF
Copy link

MNF commented Sep 3, 2018

I have a concern that “new version will break compatibility completely with version 5.”
I wil not start to use the current version, if I know that all calls will need to be changed when the new version arrives.
Existing users will hesitate to install a new version, because everything needs to be changed.

Consider to provide migration path with support of existing [Obsolete] classes and explanation why it’s better to use new classes.
Ideally old classes could be just wrappers for the new architecture.

Evolution is better than revolution. Your users should be able to upgrade to new version and later when they have time, replace their code to use the new architecture.

Another issue is that the branch was made from scratch. Will you consider to create separate product and give it version 1? It will give clear indication that product is not stable yet and users should expect new bugs and missing from v5 functionality. People expect that next version is more stable than previous.

Sorry, if my suggestions are not applicable to your library. I am just providing my general considerations, as I am not familiar with your library. I am a bit disappointed, because yesterday night I decided to try the library, but today I read that it will be rewritten from the scratch.

@tallesl
Copy link
Contributor Author

tallesl commented Sep 4, 2018

@MarkusKgit

Now that there will be individual schedulers - will there be an option to provide your own scheduler?

Now it's really easy to allow such thing, publicly exposing ITimeCalculator (which is already internally used by the library) would do the trick.

However, just being easy is terrible reason to implement something *. I'm skeptical in allowing the user to inject the scheduling logic, since that's precisely what the library is all about.

If you are doing the scheduling logic yourself, why the library?

If you are mocking such thing in your tests, what exactly are you testing?

@MNF

Existing users will hesitate to install a new version, because everything needs to be changed.

Existing users (of any piece of software) should hesitate to perform a major update. There’s a tradeoff involved: stay with stale software with no change or jump to the new thing but having to adapt.

Consider to provide migration path with support of existing [Obsolete] classes and explanation why it’s better to use new classes. Ideally old classes could be just wrappers for the new architecture.

That's the happy path, but I'm afraid it doesn't apply here (more at the end). How does one keep a global scheduler working when the goal is to precisely ditch that? What sort of frankenstein would that be?

Evolution is better than revolution.

I wouldn’t affirm that without context, and this is coming from a somewhat conservative person ;).

Another issue is that the branch was made from scratch. Will you consider to create separate product and give it version 1?

Nope. It is still a “Automated job scheduler with fluent interface for the .NET platform”. Also, I don’t want a whole repository with issues answered as “no new features here, migrate to the one over there”.

It will give clear indication that product is not stable yet and users should expect new bugs and missing from v5 functionality.

A major version is the “clear indication” of that *:

  • MAJOR version when you make incompatible API changes,
  • MINOR version when you add functionality in a backwards-compatible manner, and
  • PATCH version when you make backwards-compatible bug fixes.

As for not being stable yet, that’s why the concept of a pre-release exists (alpha), present in both GitHub and NuGet.

People expect that next version is more stable than previous.

Sorry people, but that’s the wrong expectation to have. You should expect that 5.3.0 is more stable than 5.0.0, and that 6.0.0 is less stable than 5.3.0. That's how versioning works.

I am a bit disappointed, because yesterday night I decided to try the library, but today I read that it will be rewritten from the scratch.

I would have the opposite reaction. My typical disappointment is finding software that I'm willing to use but is just left to rot out there * * *.

An end note: I'm generally on your side (and Joel's). The problem is that there's a design flaw at very core of the code base, that I've been trying to tackle (without a rewrite) since 2015. It's a lost battle. Not only I've been unable to implement features that I personally want (#34), but it's been a struggle to keep things stable (#130, #188).

@mrgfisher
Copy link

Hi @tallesl

Having checked out the redesign branch, I quite like what you are doing with the general separation of concerns, the dropping of IJob and the easy way to adopt my own IOC/DI preferences. In particular the flexibility to use either the fluent time calculator or the cron time calculator is 'quite tidy'.

I popped a unit test in place for the cron format and was dismayed to see that my unit test had to run for at least one minute - I didn't know cron only scheduled at 1 minute resolution!

There was also one small code change required for the other unit tests to run, in FluentTimeCalculator.cs
from
DateTime? ITimeCalculator.Calculate(DateTime last) => CalculateOnce(last) ?? CalculatePeriod(last);
to
public DateTime? Calculate(DateTime last) => CalculateOnce(last) ?? CalculatePeriod(last);
If you want a pull request let me know.

Many thanks for the time, thought and effort you have, and are putting into this useful library.

@tallesl
Copy link
Contributor Author

tallesl commented Sep 5, 2018

I just fixed that, thanks for pointing it out. You may have to throw out your clone and get a new one, I did a little rebase on the branch (sorry!).

You can either implement it publicly like you suggested or keep it explicit but cast it before using it.

I prefer the explicit implementation + casting in this case because I find confusing having an internal interface and implementing it with public members.

That's the language's fault by the way:

Interfaces declared directly within a namespace can be declared as public or internal and, just like classes and structs, interfaces default to internal access. Interface members are always public because the purpose of an interface is to enable other types to access a class or struct. No access modifiers can be applied to interface members.

@pujux
Copy link

pujux commented Sep 19, 2018

Well, now reading this makes my 2 recent comments invalid... I was gonna use this library to implement a "if this then that" type of wpf application but it would have to be finished by the end of the school year (probably may/june). Do you have any idea how long you might be working on this? I understand that asking this may sound like the dumbest thing one could ask, at least that's what it sounds too me, but you can also probably relate to my situation? I am also going to overthink if a scheduler library is even what I need, since working with triggers ("if this") would be more beneficial... Anyways, excellent work, I really loved playing around with the fluent syntax!

@VitorCioletti
Copy link

@puf17640 I feel your pain because I need it in a professional project. There are some small fixes we still have to do (#181 , #119 , #198), unit tests and a reentrancy issue. However, I don't think it will take much longer to publish the new version.

@pujux
Copy link

pujux commented Sep 26, 2018

@VitorCioletti Sounds great! Do you need any help? I added a Condition System, so Tasks only run if a certain Condition is met, to the library. I hope I can implement it in the new Version, because it works really good as it is now. But I'm really happy to see this is still going on and has not been forgotten.

@VitorCioletti
Copy link

@puf17640 Condition system? What motivated you to implement it?
Why don't you just put this condition in the task you are running on?

There are some missing unit tests on Cron schedule we recently added. You could implement them.

Feel free to open a pull request

:)

@pujux
Copy link

pujux commented Sep 26, 2018

@VitorCioletti Well, I'm currently working on a "If this then that" application for windows and I wanted Condition to be seperated from Task, so they can be used in however combination there is. At first I was not quite sure if this library would fit my needs, but with a few tweeks it works perfectly fine.

Okay, I might do that if I find time tomorrow! Would be my first time opening a pr on here.

@TheColonel2688
Copy link

TheColonel2688 commented Feb 13, 2019

I actually would like to but I don't have any free time (what little programming I do is at work). A piece of advice, if you like software development, stay single and don't have kids. Haha, Wife and Kids are very time-consuming.

@VitorCioletti
Copy link

@TheColonel2688 haha, got it. Thanks for the advice.

@pwen090
Copy link

pwen090 commented Feb 26, 2019

This sounds awesome guys - any plans to add exclusion support? It would be great if you could have a cron string for when to execute and a cron string for when not to execute.

@VitorCioletti
Copy link

@pwen090 There is no such thing in Cron API. However, I'm developing #227 in Fluent API that restricts scheduling throught Except(params DayOfWeek[]) function.

Please, open a new issue if you want do discuss your suggested feature.

Thanks.

@thomasd3
Copy link

Is there any ETA for the new version?

@tallesl
Copy link
Contributor Author

tallesl commented Jun 28, 2019

@thomasd3

It will be done when it's done.

I'm struggling to find free time to work on it, but it's not forgotten. I personally want to get it done before September, and I don't want to sing happy birthday to this issue.

@jotatoledo
Copy link

jotatoledo commented Jul 23, 2019

@tallesl hey there, first of all, thanks for the awesome library and the efforts being done in the refactoring process!

About the last, a couple of questions:

  • Is there any particular reason for vendoring NCrontab in the library instead of adding the package as dependency?
  • Is there an actual list of to-be-done issues for the redesign branch? I would be happy to contribute!

@kevinhillinger
Copy link

kevinhillinger commented Aug 11, 2019

@tallesl echoing the same gratitude for the work that's been put into FluentScheduler. Thank you!

I've reviewed the rewrite that's going on, and wanted to share some thoughts on approach and support for existing consumers of the library.

With v6, the API for what is FluentScheduler is essentially gone. As the creator of the library, this is, of course, your prerogative, but there are probably many thousands of instances of the library running in production that will never move to v6 because the API is essentially gone--as you've noted, zero compatibility.

I really liked FluentScheduler (currently v5) not just for its fluent scheduling but for the simple and concise abstractions it provided to structuring jobs/tasks (like IJob, etc.).

Would you be open to contributions to v5 in this repository while v6 can continue as a net new and completely rewritten library? This would allow for continued evolution and backwards compatibility of existing implementations.

@tallesl
Copy link
Contributor Author

tallesl commented Aug 19, 2019

@jotatoledo

Is there any particular reason for vendoring NCrontab in the library instead of adding the package as dependency?

Since .NET versioning is pretty much DLL hell*, I'm a bit skeptical of referencing another library somewhat popular such as NCronTab. In other words, I'm preventing another potential source of issues on my plate.

Is there an actual list of to-be-done issues for the redesign branch? I would be happy to contribute!

There's not an actual list, but it goes pretty much like this:

  • Fluent:
    • Almost finished but, from the top of my head, I don't know what's left (@VitorCioletti is implementing those).
  • Schedule:
    • Only reentrancy is missing. It may sound minor, but it's tricky to implement it without sacrificing the cleanliness (and why not, elegance) of the Run method. I have implemented it myself in my local machine, but I threw it away since I was not happy with it.
  • Documentation:

If you want to help on reentrancy, a word of warning: I'm very picky about that method. Make sure you have a deep understanding of what's going on there before attempting to change it.

There's little value on helping with documentation right now. I don't know about Vitor's stuff.

I plan to make a pre-release after what's listed above is done. All sorts of testing will be welcome!

@tallesl
Copy link
Contributor Author

tallesl commented Aug 19, 2019

@kevinhillinger

@tallesl echoing the same gratitude for the work that's been put into FluentScheduler. Thank you!

I appreciate it!

With v6, the API for what is FluentScheduler is essentially gone. As the creator of the library, this is, of course, your prerogative, but there are probably many thousands of instances of the library running in production that will never move to v6 because the API is essentially gone--as you've noted, zero compatibility.

I really liked FluentScheduler (currently v5) not just for its fluent scheduling but for the simple and concise abstractions it provided to structuring jobs/tasks (like IJob, etc.).

That's life.

The good part of FluentScheduler is still there, you can fluently setup the schedule plus there's cron support now. Yay.

Now, the bad part, the loose abstractions of registry, manager, and factory, are all gone. For good. Praise the Lord.

The API is incompatible, but it shouldn't be prohibitively difficult to port it, especially how simple the new API is. There may be some significant work on handling the lifetime of the schedule object yourself now (which is a good thing btw). But again, that's life.

Would you be open to contributions to v5 in this repository while v6 can continue as a net new and completely rewritten library? This would allow for continued evolution and backwards compatibility of existing implementations.

Bug fixes? v5 and v6.
New features? v6 only.

Keeping two repositories alive and, even worse, supporting new features on both, it would be a nightmare.

Please note that we're not rewriting it out of vanity. It's been a struggle to implement new features with the current API and its inner workings.

@VitorCioletti
Copy link

VitorCioletti commented Aug 19, 2019

@tallesl and @kevinhillinger

Hi,

There are 2 last things I have to implement:

  1. Between method and its unit tests.
  2. Calculate next execution to the closest possible. Ex: s.Every(1).Month().On(20).At(13, 00) is always going to calculate to the next month. The library must validate if is is possible to calculate the next execution in the same month (anytime before day 20 in the example).

I'm really anxious to implement these but college stuff are slowing me down.

@VitorCioletti
Copy link

Implemented Between :). I believe 2 will be hard but I already have some ideias.

@forktrucka
Copy link

What's the status of this? I'm particularly interested in the CRON support, have pulled latest nuget, and doesn't seem to match up with the hype for CRON mentioned in this thread nor the documentation?
Should i just give up and try another framework that is active/supported by someone?

@brianjlacy
Copy link

What's the status of this? I'm particularly interested in the CRON support, have pulled latest nuget, and doesn't seem to match up with the hype for CRON mentioned in this thread nor the documentation?
Should i just give up and try another framework that is active/supported by someone?

+1 @forktrucka -- I'm anxious as well.

@unruledboy
Copy link

Going through all the issues, including the closed ones, I am really confused. Is the CRON support really there? I went through unit tests etc. could not find any evidence.

@mrgfisher
Copy link

It works for me... using the redesign branch. I have all Cron tests passing and added a couple of others to test other scenarios - all looks good.

I'm also using this library in a hobby project, and it 'just works' 🥇 So I'm very happy and grateful to @tallesl for the work he has done.

@unruledboy
Copy link

@mrgfisher wow, much simpler API, nice. except the NCronTab syntax is not exactly the same as http://www.cronmaker.com/?3

@jrowe88
Copy link

jrowe88 commented Feb 12, 2020

How would you characterize the stability of the redesign branch today? I have a non-critical project and figured I'd use the latest and greatest if it is kind of close. Saw reference to 2 remaining issues back in August 2019 that aren't relevant to me.

@TheColonel2688
Copy link

Yes I too would like to know. I have an up coming project that I need to Scheduling for.

@TheColonel2688
Copy link

Could you guys do a preview/pre-release nuget?

@brianjlacy
Copy link

@tallesl I've been playing around with the new API and it's quite nice so far. I really appreciate the work you have put into this project, both the original and this new implementation.

It's also functional enough that, as suggested by @TheColonel2688, I'd consider it worthy of an "alpha" or "pre" package.

@tallesl
Copy link
Contributor Author

tallesl commented Apr 10, 2020

You guys read my mind. I just got back to work on the library and I plan to do just that this weekend.

But notice that's definitely an alpha, there's a lot of testing to be done.

@tallesl
Copy link
Contributor Author

tallesl commented Apr 15, 2020

There's an alpha version now, see #273.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests