Add JobStuckHandler callback#1291
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
brandur
force-pushed
the
brandur-stuck-job-handler
branch
from
098d829 to
93b3691
Compare
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Needs a changelog entry and one suggestion on naming, otherwise LGTM!
| // OpenWorkerSlot instructs River to treat the stuck job as no longer | ||
| // occupying a worker slot so another job can begin executing. This can be | ||
| // dangerous because the stuck job's goroutine is still running, so the queue | ||
| // may temporarily have more active job goroutines than MaxWorkers. | ||
| OpenWorkerSlot bool |
There was a problem hiding this comment.
I wonder if ReleaseWorkerSlot might be slightly clearer?
There was a problem hiding this comment.
Hmm, I think "open" is a little more correct here — "release" seems to imply that an existing slot is being released, which is not what's happening (unfortunately, we can't release the existing slot). "Open" is better IMO because it says you're "opening a new slot". That said, maybe AddWorkerSlot is better than either as it's even clearer. Changed to that.
brandur
force-pushed
the
brandur-stuck-job-handler
branch
2 times, most recently
from
b5106b0 to
e767dfe
Compare
Here, add a `JobStuckHandler` callback that's invoked when a producer consider a job to be "stuck". i.e. Passed its timeout, cancellation attempted, but job didn't respond to cancellation. The callback includes some basic information about the job that became stuck, along with the total number of stuck jobs. A result will optionally open a new executor slot to replace the one taken up by the stuck job so that a producer that continues to be run doesn't get completed starved by stuck jobs. The idea here is that clients can configure themselves to open new slots up to a certain point, but then may want to restart themselves if there's enough jobs stuck that they could become a memory liability.
brandur
force-pushed
the
brandur-stuck-job-handler
branch
from
e767dfe to
42e5151
Compare
2 participants
Here, add a
JobStuckHandlercallback that's invoked when a producerconsider a job to be "stuck". i.e. Passed its timeout, cancellation
attempted, but job didn't respond to cancellation.
The callback includes some basic information about the job that became
stuck, along with the total number of stuck jobs. A result will
optionally open a new executor slot to replace the one taken up by the
stuck job so that a producer that continues to be run doesn't get
completed starved by stuck jobs. The idea here is that clients can
configure themselves to open new slots up to a certain point, but then
may want to restart themselves if there's enough jobs stuck that they
could become a memory liability.