RFC: Add RetryAfter to dbworker.StoreOptions #13457
Conversation
In campaigns we need the ability to retry jobs multiple times. (See https://github.com/sourcegraph/sourcegraph/issues/12700#issuecomment-671798531 for additional context.) This is what I think is the easiest-to-understand and simplest solution. I did have another solution that involved a PreDequeue hook (that returned the custom conditions you see here now) and boolean in the StoreOptions to switch between AND'ing or OR'ing the custom conditions to the selectCandidateQuery. This felt a bit hacky. It was less code, but also easier to miss and misudnerstand. What do you think of this?
Codecov Report
@@ Coverage Diff @@
## main #13457 +/- ##
=======================================
Coverage 51.50% 51.51%
=======================================
Files 1496 1496
Lines 83303 83315 +12
Branches 6798 6798
=======================================
+ Hits 42905 42918 +13
Misses 36805 36805
+ Partials 3593 3592 -1
*This pull request uses carry forward flags. Click here to find out more.
|
Yeah, absolutely. I played around with this manually/interactively (prefer that over tests when doing SQL), so I can confirm it Works On My Machine At Least ™️. If you think this approach is good, though, and I shouldn't move this to somewhere else, I'm going to add tests. |
Nah I think the behavior described here is good. It may be worth it to try to merge the dequeue queries together (but maybe this is more difficult than just leaving it as-is), but I think tests should be added here first because the code is currently clear. |
| // more than RetryAfter ago. | ||
| // If RetryAfter is a non-zero duration, the store dequeues records where | ||
| // - the state is 'errored' | ||
| // - the failed attempts counter hasn't reached MaxNumResets |
There was a problem hiding this comment.
num_resets does not track failures but how many times the handling process loses the transaction holding the record (from processing -> queued). Maybe we should add a new field that increments each time it goes to errored?
There was a problem hiding this comment.
Yeah, I had the same thought: retries would be better. I'm happy to add the field.
There was a problem hiding this comment.
It can also be a post-merge enhancement since this is probably a good heuristic for now. Should definitely be done at some point but no need to pile it onto this v1 effort.
I tried this and... it wasn't nice :( |
mrnugget
force-pushed
the
mrn/worker-retry-after
branch
from
93ffcab to
c417c46
Compare
|
Added tests and changed the code to only include |
|
Not sure whether I can/should merge now, @efritz, so I'll hold off until I get explicit approval. |
In campaigns we need the ability to retry jobs multiple times. (See https://github.com/sourcegraph/sourcegraph/issues/12700#issuecomment-671798531 for additional context.)
This is what I think is the easiest-to-understand and simplest solution.
I did have another solution that involved a PreDequeue hook (that
returned the custom conditions you see here now) and boolean in the
StoreOptions to switch between AND'ing or OR'ing the custom conditions
to the selectCandidateQuery.
This felt a bit hacky. It was less code, but also easier to miss and
misudnerstand.
What do you think of this?