[GH-1623] workloads/update-workload! should define own transactions #597
Conversation
api/src/wfl/module/staged.clj
Outdated
| (patch-workload tx workload {:updated now}) | ||
| (when (every? stage/done? [source executor sink]) | ||
| (patch-workload tx workload {:finished now})) | ||
| (workloads/load-workload-for-id tx id)))] | ||
| (if (and started (not finished)) (update! workload (utc-now)) workload))) |
There was a problem hiding this comment.
It isn't new in this PR but noticed that update-sink! and patch-workload run in separate TXs?
I guess workload state will converge. (Batch updates are simpler here.)
There was a problem hiding this comment.
Yes, all of the individual stages defined transactions as needed to avoid external calls within open transactions.
But yes, there are thread safety concerns where retries are concerned:
| (catch Throwable t | ||
| (log/error "Failed to update workload" | ||
| :workload (workloads/to-log workload) | ||
| :workload (workloads/to-log workload-record) | ||
| :throwable t)))) |
There was a problem hiding this comment.
Now the initial load and the update run in separate TXs.
Would it be safer to run the load in the update's transaction?
(Not sure how those overlap in the schema.)
I prefer a separate backstop try to the nesting, but that looks correct.
I like the workload-record distinction.
There was a problem hiding this comment.
Yeah, good points all around.
I will see about punting the workload load to the update transactions. Originally I thought I needed the loaded workload to Slack on UserExceptions, but the workload record is sufficient.
I was on the fence about consolidating the backstops anyways (something about try-within-a-try is hard to track) so will un-nest.
There was a problem hiding this comment.
I can manage try-within-try on a good day, but not every day.
There was a problem hiding this comment.
Workload loading now takes place in the update's transactions, and try-within-a-try is no more.
- Load workload within workloads/update-workload! implementation transactions - Unnest the try-within-a-try in server/try-update
| (load-workload))] | ||
| (if (and started (not finished)) | ||
| (update! (load-workload)) | ||
| (load-workload))))) |
GH-1623 workloads/update-workload! should define own transactions (#597) GH-1640 Broken Terra Submission links in completed workflow Slack messages (#598) GH-1639 Pin jinja2 version for mkdocks compatibility (#596) GH-1433 No TDR temporary snapshots in tests (#595) GH-1619 Add workload info to sink logs (#594) GH-1638 Disable link unfurling when Slacking watchers (#593)
GH-1623 workloads/update-workload! should define own transactions (#597) GH-1640 Broken Terra Submission links in completed workflow Slack messages (#598) GH-1639 Pin jinja2 version for mkdocks compatibility (#596) GH-1433 No TDR temporary snapshots in tests (#595) GH-1619 Add workload info to sink logs (#594) GH-1638 Disable link unfurling when Slacking watchers (#593)
Purpose
As a prerequisite to factoring out external calls from DB transactions, this PR forces
workloads/update-workload!implementations to define their own transactions.With these changes, this decoupling is complete for staged workloads. Staged workloads can be updated in their own future in a separate PR.
Legacy workload update behavior otherwise remains the same.
System Tests
Pass:
Review Instructions
I recommend hiding whitespace when reviewing. Wouldn't mind eyes on the code consolidation I did in
wfl.server.clj.