Dedicated Queue workers per Job Type

falvarez1 commented 5 months ago

Pull request description

This PR enhances the core scheduling engine to prevent jobs of different job types from affecting each other. Some benefits are:

Improved Scheduling Accuracy: Scheduled runs occur closer to the actual scheduled time because jobs don't have to compete for the same queue.
Isolation of Long-Running Jobs: Long-running jobs of one type do not affect the scheduling and execution of jobs of other types. This resolves edge cases where concurrent long-running jobs could overload the general queue.

Additionally, this PR introduces job state notifications, job event hooks, and queue notifications for improved observability. While these features may not be immediately utilized, they will be valuable for upcoming planned work aimed at enhancing observability via a dashboard.

PR meta checklist

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

Code PR specific checklist

[x] 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.
[x] I have updated the appropriate sub section in the CHANGELOG.md.
[x] I have added, updated or removed tests to according to my changes.
[x] All tests passed.

Closes #65 Fixes #34

linkdotnet commented 5 months ago

Locally one or more tests are failing:

NCronJob.Tests test net9.0 failed with 2 error(s) (13.7s)
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:00.22]     NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCronJob [FAIL]
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobIntegrationTests.cs(301): error VSTEST1: 
      NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCro
      nJob: NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityT
      hanCronJob() Shouldly.ShouldAssertException : answer
          should be
      "INSTANT"
          but was
      "CRON"
  NCronJob.Tests test net8.0 failed with 2 error(s) (14.7s)
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:00.40]     NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCronJob [FAIL]
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobIntegrationTests.cs(301): error VSTEST1: 
      NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCro
      nJob: NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityT
      hanCronJob() Shouldly.ShouldAssertException : answer
          should be
      "INSTANT"
          but was
      "Job was canceled"

Test summary: total: 110, failed: 2, succeeded: 108, skipped: 0, duration: 14.7s

Another run:

  NCronJob.Tests test net9.0 failed with 8 error(s) (13.7s)
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:05.14]     NCronJob.Tests.NCronJobIntegrationTests.CanRunSecondPrecisionAndMinutePrecisionJobs [FAIL]
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobIntegrationTests.cs(154): error VSTEST1: 
      NCronJob.Tests.NCronJobIntegrationTests.CanRunSecondPrecisionAndMinutePrec
      isionJobs: NCronJob.Tests.NCronJobIntegrationTests.CanRunSecondPrecisionAn
      dMinutePrecisionJobs() Shouldly.ShouldAssertException : jobFinished
          should be
      True
          but was
      False
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:05.22]     NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCronJob [FAIL]
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobIntegrationTests.cs(301): error VSTEST1: 
      NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityThanCro
      nJob: NCronJob.Tests.NCronJobIntegrationTests.InstantJobHasHigherPriorityT
      hanCronJob() Shouldly.ShouldAssertException : answer
          should be
      "INSTANT"
          but was
      "Job was canceled"
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:09.26]     NCronJob.Tests.NCronJobRetryTests.CancelledJobIsStillAValidExecution [FAIL]
    /usr/local/share/dotnet/sdk/9.0.100-preview.4.24267.66/Microsoft.TestPlatform.targets(46,5): error : [xUnit.net 00:00:10.27]     NCronJob.Tests.NCronJobIntegrationTests.AdvancingTheWholeTimeShouldHaveTenEntries [FAIL]
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobRetryTests.cs(115): error VSTEST1: 
      NCronJob.Tests.NCronJobRetryTests.CancelledJobIsStillAValidExecution: NCro
      nJob.Tests.NCronJobRetryTests.CancelledJobIsStillAValidExecution() Shouldl
      y.ShouldAssertException : jobRun.JobExecutionCount
          should be
      1
          but was
      0
    /Users/stevengiesel/repos/NCronJob/tests/NCronJob.Tests/NCronJobIntegrationTests.cs(40): error VSTEST1: 
      NCronJob.Tests.NCronJobIntegrationTests.AdvancingTheWholeTimeShouldHaveTen
      Entries: NCronJob.Tests.NCronJobIntegrationTests.AdvancingTheWholeTimeShou
      ldHaveTenEntries() Shouldly.ShouldAssertException : jobFinished
          should be
      True
          but was
      False

Test summary: total: 110, failed: 4, succeeded: 106, skipped: 0, duration: 13.7s

linkdotnet commented 5 months ago

We are very close and I just getting nitpicky here ;-)

linkdotnet commented 5 months ago

LGTM!

Two last things:

Can you add the resolved issues and stuff inside the CHANGELOG.md for the release?
(Optional): Move some classes inside folders (except the builder, because I started doing that on my PR). I do feel we have a lot of objects and need some organization.

falvarez1 commented 5 months ago

2. (Optional): Move some classes inside folders (except the builder, because I started doing that on my PR). I do feel we have a lot of objects and need some organization.

Yes I feel we need to start moving things around to organize. I was hesitant to do this in this PR because it already has many changes and will make merging other ongoing PRs more difficult. I think we can take care of this in a separate, dedicated refactoring/organize PR.

Can you add the resolved issues and stuff inside the CHANGELOG.md for the release?

Will do.

linkdotnet commented 5 months ago

I think we can take care of this in a separate, dedicated refactoring/organize PR.

Yes! Feel free to merge the PR (or squash&merge if you want)

falvarez1 commented 5 months ago

@linkdotnet

I completely overlooked the fact that a forced job bypasses the queue and this cannot be "scheduled". I think I just blindly implemented the additional API methods to reflect the version that goes through the queue. But there's no way to schedule a forced instant job (atm).

Additionally I'm having second thoughts on the API name ForceRunInstantJob. It feels too verbose. The interface itself already establishes that we're working with an "instant job registry". What do you think about renaming ForceRunInstantJob to something more simple like Invoke, Trigger or ExecuteNow?

linkdotnet commented 5 months ago

Maybe for now we just scratch it (revert commit so we can recover) and tackle it later.

maybe the naming is the sole problem here. Hanford calls it „Enqueue“ which makes more sense.

There is no concept of bypassing.

I am leaning towards that we wait until customer feedback before we really implement that feature.

falvarez1 commented 5 months ago

I fixed the feature gap so instant jobs can be delayed and scheduled to run once at the desired time. I left the API method names the same and updated a variable name to better reflect its purpose.

NCronJob-Dev / NCronJob