Correct documentation on UniqueOpts.ByState#1179
Conversation
insert_opts.go
Outdated
| // The following states must be included in ByState if set: | ||
| // | ||
| // - rivertype.JobStateAvailable | ||
| // - rivertype.JobStatePending | ||
| // - rivertype.JobStateRunning | ||
| // - rivertype.JobStateScheduled | ||
| // | ||
| // These states being required is an implementation detail, but not | ||
| // requiring them would put River in a difficult position when moving jobs | ||
| // between common parts of the state machine and finding a unique conflict | ||
| // already there. |
There was a problem hiding this comment.
It's been a while since we talked about this, but I do think at least some of this is solvable with a little more work.
Edit: I see you explained a similar case below, but leaving this in place for posterity.
I'll try to illustrate one of these a little more concretely. If the only state set in this list was running, that's saying that you cannot have more than one job with these unique properties in the running state at a given time period; however, you could have other jobs that have the same unique properties in states like available or scheduled. When a client then goes to try and fetch those jobs to work them, it would be unable to transition them into the running state because it would hit a database uniqueness conflict.
The main question is what we should do in such a scenario if we were to enable that functionality. We can't have that scenario causing all job fetching on that queue to be halted. We would need to either detect and somehow handle that scenario, or allow the user to specify what should happen in such a conflicting scenario. We'd have to look a little more carefully at whether we can tell exactly which job caused the conflict when trying to fetch work, because ideally that one could be updated to a discarded state or something of that sort. The producer could then try to fetch jobs once again. That certainly would work at small scale, but it might also be really a bad idea to have that happening in a large production installation because it would crater your throughput.
So again I'll say I don't think this is an unsolvable set of problems. It's just something that had to be cut from scope in order to get this version of the unique jobs feature rolled out. It would take a fair bit of thought and additional work to try to loosen these constraints.
There was a problem hiding this comment.
If the only state set in this list was running, that's saying that you cannot have more than one job with these unique properties in the running state at a given time period
I am a newcomer to River, but reading this is surprising to me based on the rest of the documentation. UniqueOpts is part of InsertOpts and that to me means that it only controls when the job can be inserted, not how it can transition later on. For my use case (I described in #1178) I care that (transitionally) I can skip adding additional job if one is already pending, but not running. Jobs are designed so that they could run 100 of them in parallel, they are also idempotent, it is just that it is unnecessary to have more then one at any given time being scheduled to run.
About how many jobs are running in parallel, my understanding is that this is what concurrency limits are for, not insert options.
To me this is also surprising because when as a user I insert a job, it is never in any other state than an initial state. I cannot insert a job in the "running" state. That makes no sense. Only River can move job to a running state. So why would "InsertOps" control how can River move jobs between states? To me "InsertOps" is only about the user - can I as a user insert a job if any job is in a pending state but not yet running?
There was a problem hiding this comment.
In fact, to me this is a feature: River should be able to transition any available job to running state. But if River does that, then my insertion (where "running" state is not among UniqueOpts) should transactionally succeed. And this is exactly the behavior I would need. If at any given moment job is not yet running or even not being started to be run, skip adding the job. It is unnecessary. The job which will shortly run will do the job. But if the job is already running, then schedule it. Because it might be that the job running will miss a bit of work.
| // Required unique states. Requiring states like this isn't necessary | ||
| // fundamental for correctness, but doing so avoids some gnarly problems that | ||
| // don't have a clear error action otherwise. For example, if `available` was | ||
| // omittable and a producer tried to transition an `available` job to `running` | ||
| // but found another unique contender already there, we'd have to figure out | ||
| // what to do about the job that can't be scheduled. We can't send feedback to | ||
| // the caller at this point, so probably the best we could do is leave it in | ||
| // this untransitionable state until the `running` job finished, which isn't | ||
| // particularly satsifactory. |
There was a problem hiding this comment.
Well, I wrote the long explanation above before seeing this comment, where you essentially describe the same scenario. So obviously you get it :)
There was a problem hiding this comment.
Hah, yeah, I tried to write a more elaborate explanation here and then have a slightly shorter "user-friendly" one that goes on the public-facing property. The latter didn't turn out all that well — I just amended it to be a little better.
Anyway, thanks for the detailed comments here!
Try to correct some outdated documentation on `UniqueOpts.ByState`, doing my best to explain succinctly why these states are required. Fixes #1178.
brandur
force-pushed
the
brandur-correct-by-state
branch
from
21708ae to
4bfb204
Compare
3 participants
Try to correct some outdated documentation on
UniqueOpts.ByState,doing my best to explain succinctly why these states are required.
Fixes #1178.