Add task suite management endpoints and client#234
Conversation
There was a problem hiding this comment.
For the internal handling of requests in the coordinator side (code under service), I haven't reviewed them all. I think we shall discuss and decide on other things first and then we review the service code (because they are likely correct and we only need to check if we have better, more efficient and more human-readable way to write our code).
| #[arg(short = 'g', long = "group")] | ||
| pub group_name: Option<String>, | ||
| /// The UUID of the task suite to submit this task to. When set, the task is executed | ||
| /// by the suite's agents instead of traditional workers, and inherits the suite's tags. |
There was a problem hiding this comment.
Not talking about tags here. It is much more complicated than this simple expressions.
| .route("/{uuid}/close", post(close_suite)) | ||
| .route("/{uuid}/agents/include", post(include_suite_agents)) | ||
| .route("/{uuid}/agents/exclude", post(exclude_suite_agents)) | ||
| .route("/{uuid}/agents/reset", post(reset_suite_agent)) |
There was a problem hiding this comment.
I think we shall not use reset, maybe refresh? Also make all service function names more understandable.
There was a problem hiding this comment.
In theory, what is this api doing?
There was a problem hiding this comment.
This api removes the user's manual exclusion/includsion of the agent to suite so that the agent's eligibility of executing the suite will be determined by the tagmatching mechanism again.
There was a problem hiding this comment.
If that's the case, our API has some gaps. We let user to include/exlclude agent to a suite's execution list, but user can only remove them all. We should also have a remove api for that operation and that api plays a roll of removing all the relations. So you can name the two APIs from that perspective.
| /// Response for the reset-override endpoint. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct ResetSuiteAgentResp { | ||
| /// Whether a manual override existed and was cleared (false = nothing to reset). | ||
| pub reset: bool, | ||
| } |
| if verbose { | ||
| output_suite_list_info(&suite); | ||
| } else { | ||
| tracing::info!("{} ({})", suite.uuid, suite.state); |
There was a problem hiding this comment.
More summary information should be given in this one-line non-verbose output.
| return Err(Error::Custom("send register worker failed".to_string())); | ||
| } |
There was a problem hiding this comment.
Why we make these big changes? it seems only add one where clause.
|
|
||
| // Suite-bound submission: assign the task to the suite, bump its counters and reopen | ||
| // it if needed. | ||
| if let Some(suite_uuid) = suite_uuid { |
There was a problem hiding this comment.
Can we just do a match here to separate two kinds of tasks? Also, is there anything in common so that we can have some code reused?
| let labels = Vec::from_iter(labels); | ||
| let task = pool | ||
| .db | ||
| .transaction::<_, ActiveTasks::Model, crate::error::Error>(|txn| { |
There was a problem hiding this comment.
The transaction should be further optimized to make it more understandable and efficient.
2 participants
No description provided.