43 api handle semantic failures in requests #54
Conversation
If the command fails, the method now returns an error response
The method now returns a response that may indicate success or failure.
Now matching the right data structure parsed.
The method now returns a response that indicates success or failure.
Add method good() to the spawn request structure.
| optional ResponseError response_error = 1; | ||
|
|
||
| // The identifier of the newly-spawned function instance, if accepted. | ||
| optional InstanceId instance_id = 2; |
There was a problem hiding this comment.
This should be a variant (similar to a Rust result) as one never gets both an error and an id.
There was a problem hiding this comment.
Do you mean a oneof?
I am not particularly fond of that in protobuf because the C++ implementation of that is really stupid and dangerous.
Never used in Rust, we can try. Anyway, it will not guarantee that on-wire there is always at most, just that if there are two, the last one will be considered (which IMO is far less than ideal).
There was a problem hiding this comment.
Yes. Fundamentally it should be clear that there should only be one of them. Since we use Rust as the main language (and in fact the Rust API is the reference while gRPC is just one binding) I would still model that using variants (although I'm not familiar with the C++ behavior).
There was a problem hiding this comment.
OK, will do that as part of the API update following the 1st hackathon
(just FYI: in C++ the binding does not offer any special guarantee, it just leads to undefined behaviour if you do something like set(A), set(B), use(A) with A,B being in a oneof relation).
| @@ -57,6 +59,21 @@ pub struct SpawnFunctionRequest { | |||
| pub state_specification: StateSpecification, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub struct SpawnFunctionResponse { | |||
There was a problem hiding this comment.
Especially the rust representation must be a result-like object.
There was a problem hiding this comment.
OK, I can agree to that (my idea was to keep the Rust structures close to the original gRPC data structures, but there is no need for this).
There was a problem hiding this comment.
As suggested above, the Rust trait is the reference, the gRPC binding only one of possibly many bindings.
There was a problem hiding this comment.
Fine, will do that. I am not opening an issue for this, it will part of the upcoming API refactoring.
| @@ -65,7 +82,7 @@ pub struct UpdateFunctionLinksRequest { | |||
|
|
|||
| #[async_trait::async_trait] | |||
| pub trait FunctionInstanceAPI: FunctionInstanceAPIClone + Sync + Send { | |||
| async fn start(&mut self, spawn_request: SpawnFunctionRequest) -> anyhow::Result<InstanceId>; | |||
| async fn start(&mut self, spawn_request: SpawnFunctionRequest) -> anyhow::Result<SpawnFunctionResponse>; | |||
| async fn stop(&mut self, id: InstanceId) -> anyhow::Result<()>; | |||
| async fn update_links(&mut self, update: UpdateFunctionLinksRequest) -> anyhow::Result<()>; | |||
There was a problem hiding this comment.
Given this is mostly a start request this will need similar semantics.
There was a problem hiding this comment.
Do you mean for the update_links()?
If yes, then I am not sure I can agree:
Starting a new instance is something that may need a lot of resources and might have constraints. There are many reasons why one should refuse (not enough memory available, cores overloaded, annotation specifies the need for a GPU but it is already used/locked by another process, ...).
The update_links does not require more resources because it is a mere update of a forwarding table, which should not fail unless exceptional circumstances happen (I don't know, not enough memory to add few bytes needed to the data structure?).
IMO, we should use fail-semantics for start-like methods, which may be rejected in non-exceptional circumstances, while we do not need to require fail-semantics for those that may only fail in exceptional cases.
There was a problem hiding this comment.
Yes I mean update links. One can still think of some scenarios that might lead to a rejection (e.g. when violating some security policy by trying to link to some other namespace/workflow).
There was a problem hiding this comment.
I see two consistent ways of making things.
A. We implement the fail-semantics on 100% of the calls, e.g., read-only methods. That's fine, since something can always go wrong in life.
B. We implement the fail-semantics only on methods that may be willingly rejected.
I don't like A at this stage because things are moving and there will be maintenance cost to be paid.
If we go for B, as I recommend for now, then I think that update_links does not fall into the category of methods for which the node may reject that.
The violation of a security policy is something which has not been defined, as of today. When this will be a thing, then update_links will fall into the right category add we will add the fail-semantics accordingly.
I think it's more efficient to proceed on a "let's do only things that makes sense today" way and not to overdesign because our resources are very limited.
Added semantic failure to the Start method in the workflow/function/resource APIs.
For the other methods (Stop/Update/List) it is less critical to do the same since the callee should never reject the request, except for exceptional cases, such as internal server error or an invalid request.
The next step is to handle the failure on the caller side, but that's another issue ;-)