feat(plugin): Add new task-batching plugin. #495
Conversation
DrRebus
requested review from
rbeuque74,
loopfz,
rclsilver and
wI2L
as code owners
DrRebus
changed the title
DrRebus
force-pushed
the
dev/rtordjma/task-batch-plugin
branch
2 times, most recently
from
d7f70c1
to
9b076b1
Compare
DrRebus
force-pushed
the
dev/rtordjma/task-batch-plugin
branch
from
fa28ad3
to
08c3af2
Compare
| @@ -524,7 +525,8 @@ forLoop: | |||
| if mapStatus[status] { | |||
| if status == resolution.StateWaiting && recheckWaiting { | |||
| for name, s := range res.Steps { | |||
| if s.State == step.StateWaiting { | |||
| // Steps using the batch plugin shouldn't be run again when WAITING | |||
| if s.State == step.StateWaiting && s.Action.Type != pluginbatch.Plugin.PluginName() { | |||
There was a problem hiding this comment.
I'm not sure why steps are run once again when they reach the WAITING state, but it creates a race condition with the batch plugin. If the last task of the a sub-batch tries to resume its parent, but said parent is being run a second time by the engine, preventing the child task from resuming it.
Added a task-batching plugin that creates X tasks of the same template and waits until all of them are in a final state. Moved batch-creation code to the engine package and made the POST batch handler use it. Exposed final tasks' states to be used by the batch plugin. Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
…when the task belongs to a batch where other tasks are still running. Added a way of getting the amount of tasks still running in a batch (moved from the batch plugin). Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
Simplified output formatting. Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
Added a Readme. Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
DrRebus
force-pushed
the
dev/rtordjma/task-batch-plugin
branch
from
e980730
to
ddfde5b
Compare
|
|
||
| // increaseRunMax increases the maximum amount of runs of the resolution matching the given parentTaskID by the run | ||
| // count of the given batchStepName. | ||
| // Since child tasks wake their parent up when they're done, the resolution's RunCount gets incremented everytime. We |
There was a problem hiding this comment.
| // Since child tasks wake their parent up when they're done, the resolution's RunCount gets incremented everytime. We | |
| // Since child tasks wake up their paren when they're done, the resolution's RunCount gets incremented everytime. We |
There was a problem hiding this comment.
I'm pretty sure "waking [someone / something] up" is correct. 🙃
Though, both seem fine: https://dictionary.cambridge.org/fr/dictionnaire/anglais/wake-up
| query, params, err := sqlgenerator.PGsql. | ||
| Select("count (*)"). | ||
| From("task t"). | ||
| Join("batch b on b.id = t.id_batch"). |
There was a problem hiding this comment.
id_batch should be batch_id to be coherent with the rest of the code, and it reads better.
There was a problem hiding this comment.
I couldn't agree more, but id_batch is the column's name in the database, I have no power over it sadly. 🥲
https://github.com/ovh/utask/blob/master/sql/schema.sql#L44
pkg/batch/batch.go
Outdated
| return taskIDs, nil | ||
| } | ||
|
|
||
| func conjMap(common, particular map[string]interface{}) (map[string]interface{}, error) { |
There was a problem hiding this comment.
The name of this function does not clearly reflect its usage.
It does a merge/union of two map while checking that common keys are not overriden => btw, I'm skeptical of the check line 71/72. It seems legitimate to want to define a "default/common" value for all tasks, while still overriding it for some tasks, no?
There was a problem hiding this comment.
This function used to sit elsewhere (https://github.com/ovh/utask/blob/master/api/handler/batch.go#L80), I simply moved it and kept the exact same behaviour. Changing its name (something like mergeMaps seems adequate) won't have any side effect, but I'm not sure changing its behaviour is 100% safe.
That being said, allowing each task to override the default value is a good idea and seems like a valid use case.
Increased the clarity of some variables, functions and the Readme. Signed-off-by: Ruben Tordjman <ruben.tordjman.ext@ovhcloud.com>
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
A new feature: a task batching plugin.
What is the current behavior? (You can also link to an open issue here)
The only way of batching tasks so far is to use a
subtaskplugin with theforeachfeature, which dynamically adds a new step persubtask. This approach isn't efficient DB-wise, because everytime a step is updated, the whole step tree (a JSON formatted string) gets updated, triggering anUPDATEin the database. TheseUPDATEs create new bigger-and-bigger rows and delete the previous ones, bloating the DB and requiring a strongerVACUUM.What is the new behavior (if this is a feature change)?
The task-batching plugin spawns child tasks as independent tasks that wake the parent once completed. Since the parent task no longer embed its children, no hard link exists between them, but they're linked via:
ParentTaskIDtag, the public ID of the parent task, allowing the child to wake its parent.BatchIDproperty, the public ID of the batch the task belongs two.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Nope.
Other information:
This plugin doesn't yet aggregate results of the children tasks. It'll come in a subsequent version.
Contrary to what's been discussed, when using sub batches (
sub_batch_size> 0), children tasks don't always wake their parent when they're done. Instead, the last task in the sub batch will wake the parent. That way, we limit even more the amount of UPDATEs made. The downside is that we have to wait for the whole sub batch to be done before spawning more tasks in the batch.