Add protection against failure to reserve jobs

[Raveline]

WIP

[Raveline]

All right, this version works for a wrong query in the consumers. Here is an example by adding a non existing column:

  "error": "DBException {dbeQueryContext = SQL \"UPDATE chargebee_tasks SET   reserved_by = <16> , attempts = CASE     WHEN finished_at IS NULL THEN attempts + 1     ELSE 1   END WHERE id = ANY( <Array1 []> ) RETURNING id, event, attempts, surprise_surprise\", dbeBackendPid = 500778, dbeError = DetailedQueryError {qeSeverity = \"ERROR\", qeErrorCode = UndefinedColumn, qeMessagePrimary = \"column \\\"surprise_surprise\\\" does not exist\", qeMessageDetail = Nothing, qeMessageHint = Nothing, qeStatementPosition = Just 181, qeInternalPosition = Nothing, qeInternalQuery = Nothing, qeContext = Nothing, qeSourceFile = Just \"parse_relation.c\", qeSourceLine = Just 3651, qeSourceFunction = Just \"errorMissingColumn\"}, dbeCallStack = [(\"runPreparedSQL\",SrcLoc {srcLocPackage = \"consumers-2.3.4.0-83af746950b8f91e9e93ca91ee357d2eb6bad519fa73eb8edc6643cfa528cbf7\", srcLocModule = \"Database.PostgreSQL.Consumers.Components\", srcLocFile = \"src/Database/PostgreSQL/Consumers/Components.hs\", srcLocStartLine = 387, srcLocStartCol = 17, srcLocEndLine = 387, srcLocEndCol = 31})]}",

Though technically, in its current state, it says it will reenqueue tasks even if there are no tasks (and the error will pop every so often, but that's ok: it's pretty bad to have a query that fails even when there are no actual jobs to run), so I suspect we need two different handle.

[Raveline]

Latest commit also guarantees it doesn't fail when a single job fails to parse (without blocking the others). However, the update logic doesn't seem to work yet for some reason :(

[Raveline]

Build errors are unrelated (but I suspect we're missing some lib install in the CI). This works, and is tested. Now, the main issue is having all these rollbacks / commit in the middle of the consumer loop - it's not great. Another solution would be to have a savepoint, but I think it actually makes sense to rollback in this context. @arybczak any opinion or better solutions ?

[arybczak]

Another solution would be to have a savepoint

Nah, this one is good because it rollbacks only when there's an error, so the common path stays efficient. A savepoint would have to be always created, adding unnecessary overhead for each reservation.

[arybczak]

This no longer works as advertised, because if it returns ([Nothing], 1), then batch size lies and the when (batchSize > 0) $ do block above is unnecessarily entered.

I think it's best to return m [job], i.e. get rid of maybes before returning from this helper and calculate batchSize using length in loop.

[arybczak]

Why 6 hours? #39 does 1 day.

Also, this bit of the exception handler until pure is the same as below, you can make a helper function out of it.

[Raveline]

I don't remember why I changed it. I've set it back to one day.

Unfortunately, factorizing is hard here:

• If I want to factorize the rollback, GHC demands that I add MonadDB instances EVERYWHERE;
• The log messages are different, and use different objects;

So the only part I managed to really unite is the update logic - which is not nothing, since it gives us a single place to define the delay before the job gets retried.

[arybczak]

No need for explicit commit, it's going to be committed right after as we exit runDBT immediately.

[arybczak]

Suggested change

	results <- forM (F.toList qr) $ \(jobIdRow :*: other) ->
	results <- forM (F.toList qr) $ \(rawJobId :*: jobSelectors) ->

jobIdRow is a weird name, I'm not sure how it's a row. And other is non-descriptive.

Also, you can use fetchMany for less cruft, explicit queryResult in the original code was used for lazy processing, but we're forgoing it here.

[Raveline]

fetchMany demands again a cascade of constraint, so I opted to keep it as is.

[arybczak]

I'd do

(Just <$> liftBase (evaluate $ ccJobFetcher other)) `catch` \(SomeException e) -> do
   ...

seems more readable to me that way since you immediately see which code is being handled.

I also extracted Just outside the evaluate because the combination of evaluate, $ and $! isn't great for readability imo :P

[Raveline]

Much better, thank you !

[arybczak]

If you make a do block above, you won't need in for the let.

[arybczak]

Redundant, MonadMask implies it.