go-co-op / gocron

Easy and fluent Go cron scheduling. This is a fork from https://github.com/jasonlvhit/gocron
MIT License
5.59k stars 308 forks source link

[FEATURE] - Change limit at runtime #768

Open pcfreak30 opened 2 months ago

pcfreak30 commented 2 months ago

I have run into the need to set the runtime limit for the cron and want input on what I did before I make a PR.

https://github.com/go-co-op/gocron/compare/v2...LumeWeb:gocron:a0bc9ad53805a261cf60a87414e8d3ee734c6632

Since the re-schedule mode uses a chan and they cannot be resized, and trying to "move" entries between chans might be messy? I just added a check and new error object for it.

Kudos.

pcfreak30 commented 2 months ago

Update:

Also had to expose limitModeConfig

https://github.com/LumeWeb/gocron/commit/2ecb4bdb516997f3823ab8093f68fc9a37be8f05

JohnRoesler commented 2 months ago

This is altering the limit mode on the scheduler after it's been set up, right?

To support this, I'd want to see done in a manner similar to how the job has an Update method. The scheduler could have an update method, that would allow then passing options similar to initial creation. If possible, it could just be all the same options. If there are too many complications with that, there could be a lesser set of update-able options that could be passed in, e.g. WithLimitConcurrentJobs

pcfreak30 commented 2 months ago

This is altering the limit mode on the scheduler after it's been set up, right?

To support this, I'd want to see done in a manner similar to how the job has an Update method. The scheduler could have an update method, that would allow then passing options similar to initial creation. If possible, it could just be all the same options. If there are too many complications with that, there could be a lesser set of update-able options that could be passed in, e.g. WithLimitConcurrentJobs

Yes. though im not sure what your referring to. Would appreciate if you can clarify or give an example of what your thinking?

(also ill be submitting a PR with all changes im doing outside this too, just many scattered priorities ATM)

JohnRoesler commented 2 months ago
func NewScheduler(options ...SchedulerOption) (Scheduler, error) {}

type scheduler struct {
    ...
}

func (s *scheduler) Update(options ...SchedulerOption) error {}
pcfreak30 commented 2 months ago
func NewScheduler(options ...SchedulerOption) (Scheduler, error) {}

type scheduler struct {
    ...
}

func (s *scheduler) Update(options ...SchedulerOption) error {}

Only concern is if that would break anything because some things cannot be edited at runtime safely in all cases? Which might add complexity in trying to do that.

I additionally still need a getter for the Limit mode so i can reference it for task coordination, so want to know if you have an issue with that API change?

pcfreak30 commented 3 weeks ago
func NewScheduler(options ...SchedulerOption) (Scheduler, error) {}

type scheduler struct {
    ...
}

func (s *scheduler) Update(options ...SchedulerOption) error {}

Only concern is if that would break anything because some things cannot be edited at runtime safely in all cases? Which might add complexity in trying to do that.

I additionally still need a getter for the Limit mode so i can reference it for task coordination, so want to know if you have an issue with that API change?

@JohnRoesler I am spending time on this again in my project, would like to know your response. Thanks.

JohnRoesler commented 3 weeks ago

Yes, I am ok with adding a Getter for limit mode.

I'd still like to continue exploring the Update method on the Scheduler path. I do agree there are some complications and that not all methods should be available for Update.

Perhaps rather than having it be ...SchedulerOption, it could be a new set, ...SchedulerUpdateOption and then we could have that be a wrapper basically around schedulerOption and only expose the ones that are allowed to be updated