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

No Restriction on Jobs with Same Key #22

Closed
Hoteliar opened this issue Jan 11, 2022 · 5 comments · Fixed by #84
Closed

No Restriction on Jobs with Same Key #22

Hoteliar opened this issue Jan 11, 2022 · 5 comments · Fixed by #84

Comments

@Hoteliar
Copy link

Tried to insert 2 jobs with same key (int), both of them were inserted. But the GetScheduledJob by ID only returns one job.
I expect the second will override the first one.

@pires
Copy link

pires commented Jan 12, 2022

IIUC the same problem happens if you delete by ID, where only the first Job found w/ said ID is deleted but others stay scheduled. That said, I'm currently thinking ensuring only one job with a certain ID is present by always overwriting seems like the sane thing to do.

@reugn
Copy link
Owner

reugn commented Jan 26, 2022

The original Java Quartz throws an exception on a duplicate job being scheduled. The current architecture would require a full scan of all scheduled jobs to detect duplicates, which is inefficient. It's possible to explicitly call the DeleteJob before scheduling a job to check for uniqueness. But this requires the user to be aware of the possible problem with duplicating jobs.

@pires
Copy link

pires commented Jan 28, 2022

Could DeleteJob return the deleted instance? This way one could:

job, _ := DeleteJob(id)
for job != nil {
    job, _ = DeleteJob(id)
}
// no jobs w/ ID=id

Note: for the sake of making my point in a succinct way, I'm not handling errors.

If this doesn't look crazy, it could even be added as an helper to this lib, eg ClearJob(id)?

@reugn
Copy link
Owner

reugn commented Jan 28, 2022

Yes, this one would work, but it's odd and wrong to have this functionality hidden from the user. In the meantime, the only error returned is the "No Job found" which could be an indicator. I would keep this open to wait for proper refactoring to handle the job duplication issue.

@pires
Copy link

pires commented Jan 31, 2022

Maybe a stop-gap solution could be to comment the function definitions to warn about this and maybe point to this issue as a TODO?

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

Successfully merging a pull request may close this issue.

3 participants