[Swarming] Adds Response proto & improves "unscheduled task" logic#5289
[Swarming] Adds Response proto & improves "unscheduled task" logic#5289IvanBM18 wants to merge 16 commits into
Conversation
Co-authored-by: Titouan Rigoudy <titouan@chromium.org>
IvanBM18
added
the
swarming
| @@ -29,15 +29,15 @@ | |||
| from google.protobuf import timestamp_pb2 as google_dot_protobuf_dot_timestamp__pb2 | |||
There was a problem hiding this comment.
Changes autogenerated by the protos/genreate.sh script
|
|
||
| global___NewTaskRequest = NewTaskRequest | ||
|
|
||
| @typing_extensions.final |
There was a problem hiding this comment.
Changes autogenerated by the protos/genreate.sh script
| if not swarming.is_swarming_task(task.job_type): | ||
| unscheduled_tasks.append(task) | ||
| continue | ||
| if not swarming.is_swarming_task(task.job_type): |
There was a problem hiding this comment.
Note, this is marked as new lines because i moved the try downwards to line 133, so that we avoid unnecesay nesting,since the only method that really throws exceptions is located at line 134
| os_val = self._get_dimension(task_req, 'os') | ||
| pool_val = self._get_dimension(task_req, 'pool') | ||
| if not os_val or not pool_val: | ||
| logs.warning(f'[Swarming] Failed to find required dimension for job ' |
There was a problem hiding this comment.
why did you change to warning level? this looks like error level to me
There was a problem hiding this comment.
I think what Ivan is doing is this flow is whenever an error is detected he raises an Exception, rather than logging it as an error, apparently what happens after this part is that the job is added to "unscheduled" tasks, my question would be, what happens with unscheduled tasks later on the flow?
There was a problem hiding this comment.
When the task is unscheduled it just returns to the queue.
i moved this to a warning because:
- Errors should be things that we should immediately fix, warnings are just to keep note of them but they don't brake anything, in this case a non scheduled job doesn't break anything, but its worth noting
- We already have the logs polluted with to many errors
fernandofloresg
left a comment
fernandofloresg
left a comment
There was a problem hiding this comment.
Other than Edgar's comment LGTM
3 participants
Overview
It was request for me to add a log to easily debug when swarming scheduled a task, but i noticed we didnt had the protos for parsing the response, hence i added them.
Also this PR handles an edge case in which swarming will not return a task_id if the tasks wasn't scheduled due to swarming related issues(not request, nor pc contract).
Background
This PR comes from a discussion & agreement to a required change from this PR
Changes
protos/generate.shwhich updated a ton of filescount_tasks()api method, now thepush_task()will also only raiseSwarmingAPIErrors