Notify banned solvers #3262
Notify banned solvers #3262
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
# Conflicts: # crates/autopilot/src/domain/competition/participation_guard/db.rs # crates/autopilot/src/domain/competition/participation_guard/mod.rs # crates/autopilot/src/run.rs
# Conflicts: # crates/autopilot/src/domain/competition/participation_guard/db.rs
squadgazzz
force-pushed
the
notify-banned-solvers
branch
from
640f68f to
43e650f
Compare
crates/autopilot/src/domain/competition/participation_guard/db.rs
Outdated
Show resolved
Hide resolved
| tokio::task::spawn(future.in_current_span()); | ||
| } | ||
|
|
||
| pub async fn notify( |
There was a problem hiding this comment.
why two functions with the same name but one public and the other private? is the public one really needed? 🤔
There was a problem hiding this comment.
Yes, it is needed. The public one is used in the API router. notify_and_forget is used in many other places. The private notify_ is used to define the common code, which is used in both the public notify and notify_and_forget functions. This avoids the code duplication. Not sure why it is not clear from the code. Would be happy to receive suggestions to improve this.
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error("Unable to notify solver")] | ||
| UnableToNotify, |
There was a problem hiding this comment.
Maybe it is worth to add some context information like solver address or name?
There was a problem hiding this comment.
The caller knows for which solver the request was sent. IMO, it is a redundant info here.
crates/autopilot/src/domain/competition/participation_guard/db.rs
Outdated
Show resolved
Hide resolved
crates/driver/src/infra/api/routes/notify/dto/notify_request.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { |
There was a problem hiding this comment.
nit: Personally I would be ok with omitting completely the error case for notify endpoint (be consistent with how the driver propagates it to solvers) as we always considered this endpoint not too important for system to work and we were not interested to act on failure of notification.
There was a problem hiding this comment.
Since in another PR there will be another reason, wouldn't the solver be interested in what exactly did we detect?
…fy-banned-solvers
This is a follow-up to #3257, which was requested in #3257 (comment). Implements solver notification mechanism in case the driver is banned. Reroutes the notification request to the solver through the internal driver. External solvers need to be communicated about this change.