Batch planning of sub tasks #214
Conversation
|
ACK on the approach. Please proceed with finishing the PRs |
| end | ||
|
|
||
| def run_progress | ||
| if counts_set? && output[:total_count] > 0 |
There was a problem hiding this comment.
Shouldn't we use total_count method in the condition?
There was a problem hiding this comment.
The condition was there just to refrain from dividing by zero, this shouldn't happen anymore, removed it
| BATCH_SIZE=10 | ||
|
|
||
| # Should return a slice of size items starting from item with index from | ||
| def entries(from, size) |
There was a problem hiding this comment.
What about calling this method simply batch?
|
The builds are failing on rubocop |
|
While testing, I've noticed the current implementation plans the next batch once some plans finished: we can do better just by making sure we send the event to plan next batch sooner https://gist.github.com/iNecas/4ad20c1451f46ef91b198ed5e9fdd413 |
There was a problem hiding this comment.
We don't need to wait for some plans to finish before we proceed with planning of next batch, see https://gist.github.com/iNecas/4ad20c1451f46ef91b198ed5e9fdd413
|
Cancelling should now work properly. I'm afraid the concurrency control after executor's restart never worked. I'll create a new PR for it |
|
|
||
| # Returns the items in the current batch | ||
| def current_batch | ||
| start_position = output[:total_count] |
There was a problem hiding this comment.
I suggest keeping the total_count in output to match the total_count method, and introduce planned_count to keep the number of tasks we have planned: The total_count is misleading.
| module Action::WithBulkSubPlans | ||
| include Dynflow::Action::Cancellable | ||
|
|
||
| BATCH_SIZE = 10 |
There was a problem hiding this comment.
Let's rename this to DEAFAULT_BATCH_SIZE, as it can be changed by overriding batch_size method. Also, I would recommand making default a bit higher (even 100) should be safe for most cases, when not doing too much in the planning phase.
lib/dynflow/action/with_sub_plans.rb
Outdated
| @@ -26,16 +26,20 @@ def run(event = nil) | |||
| end | |||
|
|
|||
| def initiate | |||
| sub_plans = create_sub_plans | |||
| sub_plans = Array[sub_plans] unless sub_plans.is_a? Array | |||
| output.update(:total_count => 0, | |||
There was a problem hiding this comment.
Do we need to reset the numbers here? It would break the done? method when called sooner than any planning happens. When we go with planned_count number, we should not need to reset this numbers at the beginning (just initiating the planned_count in the WithBulkSubPlans
|
Updated |
adamruzicka
force-pushed
the
batch-sub-tasks
branch
from
f68280d
to
497b2c2
Compare
|
Tests are faillnig |
|
There are still some tasks/rex changes to be made, but this one should be good to go. Thanks @adamruzicka |
|
dynflow-0.8.22 with this code pushed to rubygems |
Quickstart Guide
::Dynflow::Action::WithBulkSubPlansmodule into the action.total_countmethod returning the total count of plans we want to createentries(FROM, SIZE)method returning the batch starting at positionFROMand spanningSIZEitemsBATCH_SIZEconstant can be redefined to change the size of the batchcurrent_batchinsidecreate_sub_plansto get the current batchTODO