search

kagkarlsson / db-scheduler

Persistent cluster-friendly scheduler for Java
Apache License 2.0
1.25k stars 191 forks source link

feat: support multiple cron-styles #386

Closed tom-mckenzie-nuix closed 1 year ago

tom-mckenzie-nuix commented 1 year ago

…pe enum

Brief, plain english overview of your changes here

Allow CronSchedule to use any type of cron expression supported by CronType enum

Fixes

(https://github.com/kagkarlsson/db-scheduler/issues/384)

Reminders

cc @kagkarlsson

kagkarlsson commented 1 year ago

Nice 👍. Quick comment. We need our own crontype enum, cannot expose the one from cronutils

kagkarlsson commented 1 year ago

A test perhaps?

tom-mckenzie-nuix commented 1 year ago

Nice 👍. Quick comment. We need our own crontype enum, cannot expose the one from cronutils

Isn't the cronutils one already exposed via CronDefinitionBuilder.instanceDefinitionFor(CronType)?

tom-mckenzie-nuix commented 1 year ago

A test perhaps?

Added tests for quartz and unix style crons.

kagkarlsson commented 1 year ago

Isn't the cronutils one already exposed via CronDefinitionBuilder.instanceDefinitionFor(CronType)?

I would like to keep the underlying cron-library an implementation detail, i.e. classes and packages should not leak into the "api" of db-scheduler.

Basically what I am asking for is a duplication of CronType into the scheduler packages, and a translation between that and cronutils.

tom-mckenzie-nuix commented 1 year ago

Isn't the cronutils one already exposed via CronDefinitionBuilder.instanceDefinitionFor(CronType)?

I would like to keep the underlying cron-library an implementation detail, i.e. classes and packages should not leak into the "api" of db-scheduler.

Basically what I am asking for is a duplication of CronType into the scheduler packages, and a translation between that and cronutils.

Ah, I had missed that CronDefinitionBuilder was part of the other library. The change is in now.

tom-mckenzie-nuix commented 1 year ago

Looking good, thanks 👍

Would it be possible for you to verify that deserializing a CronSchedule serialized by the latest released version of db-scheduler works?

This does not work.

Should I make a new class for my changes and deprecate CronSchedule instead of modifying it?

kagkarlsson commented 1 year ago

Hmm, you sure? I thought it would be, after serialVersionId was set to 1 at least

tom-mckenzie-nuix commented 1 year ago

Hmm, you sure? I thought it would be, after serialVersionId was set to 1 at least

Ah, I had been working off version 11.7 which didn't have a hard-coded serialVersionUID. Going from 12.1.0 to my branch does work.

kagkarlsson commented 1 year ago

Ok, good! Thanks for testing 👍

kagkarlsson commented 1 year ago

🎉 This issue has been resolved in v12.2.0 (Release Notes)