Add capability to schedule periodic jobs dynamically #288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandur
force-pushed
the
brandur-dynamic-periodic-jobs
branch
from
4fd4c50
to
85ceb9c
Compare
brandur
commented
Mar 27, 2024
| @@ -0,0 +1,189 @@ | |||
| package river | |||
There was a problem hiding this comment.
Renamed this from periodic.go to periodic_job.go to be a bit more descriptive.
brandur
commented
Mar 27, 2024
| // leader because only the leader enqueues periodic jobs. To make sure that a | ||
| // new periodic job is fully enabled or disabled, it should be added or removed | ||
| // from _every_ active River client across all processes. | ||
| func (b *PeriodicJobBundle) Remove(periodicJobHandle rivertype.PeriodicJobHandle) { |
There was a problem hiding this comment.
Debated on whether this should return (1) nothing, (2) an error, (3) an OK boolean, or even (4) panic when a job handle isn't found.
Decided to go with no return value because I figure an unknown job will be an edge most people don't have to handle, and it mirrors the Go API of delete(map, key).
bgentry
approved these changes
Mar 27, 2024
periodic_job.go
Outdated
| // leader because only the leader enqueues periodic jobs. To make sure that a | ||
| // new periodic job is fully enabled or disabled, it should be added or removed | ||
| // from _every_ active River client across all processes. | ||
| func (b *PeriodicJobBundle) RemoveManyAll(periodicJobHandles []rivertype.PeriodicJobHandle) { |
There was a problem hiding this comment.
Any particular reason for the All suffix? I feel like RemoveMany would suffice here.
There was a problem hiding this comment.
Damn, good catch. Totally a refactor problem where I renamed something, but apparently left an outdated artifact in there. That was close ...
| t.Run("NoJobsConfigured", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| svc, _ := setup(t) | ||
|
|
||
| svc.periodicJobs = []*PeriodicJob{} | ||
| // TODO: How about just don't configure the jobs in the first place. |
There was a problem hiding this comment.
Yep, oops. I ended up changing the setup function to not add default jobs, but forgot to fix this.
Here, attempt to resolve #242 by providing a way for new periodic jobs to be added dynamically after a client's already started (i.e. not only by passing them to the client's initial constructor). The new API puts all functions on a separate `PeriodicJobs` bundle to avoid further polluting `Client`'s namespace, and to make all related functions easy to find. riverClient.PeriodicJobs().Add( river.NewPeriodicJob( river.PeriodicInterval(15*time.Minute), func() (river.JobArgs, *river.InsertOpts) { return PeriodicJobArgs{}, nil }, nil, ), ) Additions return a periodic job "handle" which can be used to remove an added job: periodicJobHandle := riverClient.PeriodicJobs().Add(...) riverClient.PeriodicJobs().Remove(periodicJobHandle) I used a handle because the `river.PeriodicJob` construct isn't particularly pretty to keep around, and contains a number of fields that are aren't comparable so its unsuitable for use in equality checks or as a map key. Adding a new periodic job bumps the enqueuer's run loop so that it enqueues the job immediately if it's configured with `RunOnStart`, and schedules its initial target run time. In other words, adding or removing periodic jobs should take effect ~instantly. Fixes #242.
brandur
force-pushed
the
brandur-dynamic-periodic-jobs
branch
from
85ceb9c
to
785c353
Compare
|
Thanks! |
brandur
added a commit
that referenced
this pull request
Mar 28, 2024
Tees up the project for version `v0.2.0`. Contains a few small changes and bug fixes, and with a new periodic job feature from #288.
Merged
brandur
added a commit
that referenced
this pull request
Mar 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here, attempt to resolve #242 by providing a way for new periodic jobs
to be added dynamically after a client's already started (i.e. not only
by passing them to the client's initial constructor).
The new API puts all functions on a separate
PeriodicJobsbundle toavoid further polluting
Client's namespace, and to make all relatedfunctions easy to find.
Additions return a periodic job "handle" which can be used to remove an
added job:
I used a handle because the
river.PeriodicJobconstruct isn'tparticularly pretty to keep around, and contains a number of fields that
are aren't comparable so its unsuitable for use in equality checks or as
a map key.
Adding a new periodic job bumps the enqueuer's run loop so that it
enqueues the job immediately if it's configured with
RunOnStart, andschedules its initial target run time. In other words, adding or
removing periodic jobs should take effect ~instantly.
Fixes #242.