Conversation
Added a function Cron.UpdateSchedule(id, spec) that can be used to update the schedule of the cron job associated with the given id. The following changes/additions were made. 1. To allow for constant time lookup, added a new member to Cron. entryLookupTable map[EntryID]*Entry 2. Added a new function, addNewEntry(*Entry) that not only appends the entry to the list of entries but also adds it to the entryLookupTable. 3. Retrofitted Cron.Schedule() and Cron.run() to now use Cron.addNewEntry() instead of directly appending Cron.entries. 4. Retrofitted Cron.removeEntry() to also delete the entry from entryLookupTable. Added test coverage for cron.UpdateSchedule(id, spec). The following tests were added. 1. Update the schedule without calling cron.Start() and verify if the schedule interval changed. 2. Update the schedule while the cron job is running (after calling cron.Start()) and verify if the schedule interval changed.
cron.go
Outdated
| nextID EntryID | ||
| jobWaiter sync.WaitGroup | ||
| entries []*Entry | ||
| // entriesLookupTable can be used for constant time lookup of entries. |
There was a problem hiding this comment.
entriesLookupTable is not necessary for your change, so I'd prefer to leave it out. Storing entries in a data structure other than a slice is a possible area for improvement, but it's not in scope for this change and has other considerations.
cron.go
Outdated
| return errors.Wrap(err, fmt.Sprintf("could not update schedule for job %d", id)) | ||
| } | ||
| if entry, ok := c.entryLookupTable[id]; ok { | ||
| entry.Schedule = schedule |
There was a problem hiding this comment.
This is a data race, because there is no synchronization between this and run(). (Is it caught by go test -race?) I think you will have to take a similar approach as in Schedule() and send the update on a channel if the cron is already running.
There was a problem hiding this comment.
You are right! I have fixed this.
cron_test.go
Outdated
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "github.com/stretchr/testify/assert" |
cron.go
Outdated
| return entry.ID | ||
| } | ||
|
|
||
| func (c *Cron) UpdateSchedule(id EntryID, spec string) error { |
There was a problem hiding this comment.
I think we may need to offer a way to Update using a Schedule instead of just a spec string, similar to how you can add a job using either.
Perhaps named Update and UpdateSchedule ?
There was a problem hiding this comment.
Makes sense. I have added in the support for the same.
cron.go
Outdated
| if entry, ok := c.entryLookupTable[id]; ok { | ||
| entry.Schedule = schedule | ||
| // Updating entry.Next as it might now be stale. | ||
| entry.Next = entry.Schedule.Next(c.now()) |
There was a problem hiding this comment.
This doesn't work -- you can verify that by adding a job with a schedule that doesn't fire for a long time, and then update it to fire immediately. It will not be run. If cron is running already, it is blocked until the next time a schedule was active, expected to be at the beginning of the entries slice. That has to be recalculated when an update happens.
I think that also points to gaps in the test coverage.
There was a problem hiding this comment.
Good catch :) I have now made sure that the entries are reordered and that the timer is reinitialized so that the updated schedule is accommodated.
Made sure that the new schedule is forwarded via channels to ensure that there isn't any race condition. After updating entry.Schedule and entry.Next, the entries are reordered as entries[0] might no longer be the next entry to run. With this, the timer is also reinitialized. Got rid of entriesLookupTable as it is not necessary for this change. Added support for passing either a spec string or a Schedule when updating the schedule of an entry. Updated test code to test that the schedule indeed was updated and that the timer was reinitialized. Got rid of the following dependencies. 1. github.com/stretchr/testify/assert 2. github.com/pkg/errors Ran "go test" and "go test -race" on the project.
…dykaushik/cron into feature/update-cron-schedule
|
@robfig I have made the changes you requested. |
|
@robfig - this is a gentle reminder for reviewing this PR :). |
|
@robfig let me know if you would want me to update the documentation to include |
|
when people can use this method? ;) |
|
Hi @robfig |
|
Bump. |
|
why don't merge to master? |
|
@pradykaushik I made a fork that addresses almost all issues of this repo. My fork keeps the entries in a heap and hashmap for fast lookup It also have the UpdateSchedule & UpdateScheduleWithSpec methods |
7 participants
Added a function Cron.UpdateSchedule(id, spec) that can be used
to update the schedule of the cron job associated with the given id.
The following changes/additions were made.
To allow for constant time lookup, added a new member to Cron.
entryLookupTable map[EntryID]*Entry
Added a new function, addNewEntry(*Entry) that not only appends
the entry to the list of entries but also adds it to the
entryLookupTable.
Retrofitted Cron.Schedule() and Cron.run() to now use
Cron.addNewEntry() instead of directly appending Cron.entries.
Retrofitted Cron.removeEntry() to also delete the entry from
entryLookupTable.
Added test coverage for cron.UpdateSchedule(id, spec).
The following tests were added.
the schedule interval changed.
cron.Start()) and verify if the schedule interval changed.