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.

Sign up for GitHub

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

Jump to bottom

Ensure multiples schedules of the same job with different chains of dependent jobs are properly processed #124

Merged
merged 1 commit into from
Nov 3, 2024
Merged

Ensure multiples schedules of the same job with different chains of dependent jobs are properly processed #124

merged 1 commit into from
Nov 3, 2024

Conversation

nulltoken
Copy link
Collaborator

@nulltoken nulltoken commented Nov 2, 2024 •
edited
Loading

Pull request description

This partly fixes #108 with regards to not properly honoring different dependent schedules.

However, this doesn't deal with the other broader concerns (for instance, how to selectively run an instant job targeting a specified configured schedule among other ones).

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.

@coveralls
Copy link

coveralls commented Nov 2, 2024 •
edited
Loading

Coverage Status

coverage: 84.767% (+0.5%) from 84.31%
when pulling 8fba306 on nulltoken:ntk/different_deps_schedules
into e6e4053 on NCronJob-Dev:main.

@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch 4 times, most recently from a1f99b7 to a4cee5c Compare November 2, 2024 13:07
@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from a4cee5c to c5a17da Compare November 2, 2024 13:25
@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from c5a17da to a7276b8 Compare November 2, 2024 16:00
@linkdotnet
Copy link
Member

One test seems to be stuck:
image

@linkdotnet
Copy link
Member

Locally I can't reproduce the hanging test, but when running via Github Codespace, it also sometimes hangs. Just doesn't show the current executed test.

@nulltoken
Copy link
Collaborator Author

Locally I can't reproduce the hanging test, but when running via Github Codespace, it also sometimes hangs. Just doesn't show the current executed test.

On it.

@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from 7a3e76a to be7a083 Compare November 2, 2024 19:42
@nulltoken
Copy link
Collaborator Author

Locally I can't reproduce the hanging test, but when running via Github Codespace, it also sometimes hangs. Just doesn't show the current executed test.

On it.

Can't repro either.
Rebased and added some xunit configuration to list hanging tests in the future.

@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from be7a083 to 1eafc1b Compare November 2, 2024 20:28
@linkdotnet
Copy link
Member

I'll try to reproduce via the Codespace. Just FYI: You can also use codespaces with 120 CPU hours for free.

@linkdotnet
Copy link
Member

It happes on the Codespace only on net9.0:
image

CanBuildAChainOfDependentJobsThatRunAfterOneJob

@nulltoken
Copy link
Collaborator Author

It happes on the Codespace only on net9.0: <img alt="image" width="1165" src="https://private-user-

CanBuildAChainOfDependentJobsThatRunAfterOneJob

Thanks for the identification. I'll take a look at it tomorrow.

@nulltoken
Copy link
Collaborator Author

It happes on the Codespace only on net9.0: <img alt="image" width="1165" src="https://private-user-
CanBuildAChainOfDependentJobsThatRunAfterOneJob

Thanks for the identification. I'll take a look at it tomorrow.

Actually, couldn't resist. So I took a quick peek.

The hanging problem seems unrelated to this PR as it can be reproduced on Codespaces against the main branch.

@linkdotnet
Copy link
Member

It happes on the Codespace only on net9.0: <img alt="image" width="1165" src="https://private-user-
CanBuildAChainOfDependentJobsThatRunAfterOneJob

Thanks for the identification. I'll take a look at it tomorrow.

Actually, couldn't resist. So I took a quick peek.

The hanging problem seems unrelated to this PR as it can be reproduced on Codespaces against the main branch.

Interesting! Thanks for the heads up! Well - probably it is fine if we tackle it on main then.

@nulltoken nulltoken mentioned this pull request Nov 2, 2024
Open
@nulltoken
Copy link
Collaborator Author

Interesting! Thanks for the heads up! Well - probably it is fine if we tackle it on main then.

Tracked through #125

linkdotnet
linkdotnet reviewed Nov 2, 2024
View reviewed changes
src/NCronJob/Registry/JobRegistry.cs Outdated
@@ -19,7 +21,7 @@ internal sealed class JobRegistry
public JobDefinition GetJobDefinition<T>() => allJobs.First(j => j.Type == typeof(T));

public JobDefinition? FindJobDefinition(Type type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the "TODO" itself. In its current state we only have a jobdefinition.

Copy link
Collaborator Author

@nulltoken nulltoken Nov 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that context, how about replacing FirstOrDefault() by SingleOrDefault()?

This would enforce the expected intent of the design and would allow to not swallow potential issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a big fan of "fail fast". Do we have an option for v3 to throw an exception when we add a duplicate?
In v4 UseNCronJob could be our savior - but not sure what can be done for v3.

If there isn't anything, maybe a custom exception might be more appropriate. Not sure what a user can do if he receives the InvalidOperationException with "Sequence contains more than one element" message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've eventually only removed the TODO.

The code actually doesn't allow duplicates. In that context, Using FirstOrDefault() or SingleOrDefault() is similar.

However, the way the code deals with duplicates isn't uniform, from a user standpoint. A such, I've created #128 (which I think should be dealt with in the context of v4 as it will certainly generate a change the behavior).

@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from 1eafc1b to 95f8209 Compare November 3, 2024 18:59
@nulltoken nulltoken changed the title [WIP] Ensure different dependent schedules are honored Ensure different dependent schedules are honored Nov 3, 2024
@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from 95f8209 to ce12546 Compare November 3, 2024 19:06
@nulltoken nulltoken requested a review from linkdotnet November 3, 2024 19:09
@nulltoken nulltoken force-pushed the ntk/different_deps_schedules branch from ce12546 to 8fba306 Compare November 3, 2024 19:47
@nulltoken
Copy link
Collaborator Author

Changelog has been updated as well.

@nulltoken nulltoken changed the title Ensure different dependent schedules are honored Ensure multiples schedules of the same job with different chains of dependent jobs are properly processed Nov 3, 2024
@nulltoken nulltoken merged commit b906e8b into NCronJob-Dev:main Nov 3, 2024
3 checks passed
@nulltoken nulltoken deleted the ntk/different_deps_schedules branch November 3, 2024 20:13
@linkdotnet
Copy link
Member

Good work! I will directly make a new release!

This was referenced Nov 3, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linkdotnet linkdotnet linkdotnet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MaybeBug/MaybeDoc] Unclear expectations about dependent jobs
3 participants
@nulltoken @coveralls @linkdotnet