-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
43 api handle semantic failures in requests #54
Changes from all commits
ecc8461
25919ca
4c32ca3
6fd3553
cd9f565
6ffaecf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#[derive(Clone, Debug)] | ||
pub struct ResponseError { | ||
pub summary: String, | ||
pub detail: Option<String>, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
use crate::common::ResponseError; | ||
|
||
// TODO(raphaelhetzel) These should be actual types in the future to allow for type-safety. | ||
pub type NodeId = uuid::Uuid; | ||
pub type NodeLocalComponentId = uuid::Uuid; | ||
|
@@ -57,6 +59,21 @@ pub struct SpawnFunctionRequest { | |
pub state_specification: StateSpecification, | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
pub struct SpawnFunctionResponse { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Especially the rust representation must be a result-like object. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine, will do that. I am not opening an issue for this, it will part of the upcoming API refactoring. |
||
pub response_error: Option<ResponseError>, | ||
pub instance_id: Option<InstanceId>, | ||
} | ||
|
||
impl SpawnFunctionResponse { | ||
pub fn good(instance_id: crate::function_instance::InstanceId) -> Self { | ||
Self { | ||
response_error: None, | ||
instance_id: Some(instance_id), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
pub struct UpdateFunctionLinksRequest { | ||
pub instance_id: Option<InstanceId>, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this is mostly a start request this will need similar semantics. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The violation of a security policy is something which has not been defined, as of today. When this will be a thing, then 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. |
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
pub struct CommonConverters {} | ||
|
||
impl CommonConverters { | ||
pub fn parse_response_error(api_request: &crate::grpc_impl::api::ResponseError) -> anyhow::Result<crate::common::ResponseError> { | ||
Ok(crate::common::ResponseError { | ||
summary: api_request.summary.to_string(), | ||
detail: api_request.detail.clone(), | ||
}) | ||
} | ||
|
||
pub fn parse_instance_id(api_id: &crate::grpc_impl::api::InstanceId) -> anyhow::Result<crate::function_instance::InstanceId> { | ||
Ok(crate::function_instance::InstanceId { | ||
node_id: uuid::Uuid::parse_str(&api_id.node_id)?, | ||
function_id: uuid::Uuid::parse_str(&api_id.function_id)?, | ||
}) | ||
} | ||
|
||
pub fn serialize_response_error(crate_function: &crate::common::ResponseError) -> crate::grpc_impl::api::ResponseError { | ||
crate::grpc_impl::api::ResponseError { | ||
summary: crate_function.summary.clone(), | ||
detail: crate_function.detail.clone(), | ||
} | ||
} | ||
|
||
pub fn serialize_instance_id(instance_id: &crate::function_instance::InstanceId) -> crate::grpc_impl::api::InstanceId { | ||
crate::grpc_impl::api::InstanceId { | ||
node_id: instance_id.node_id.to_string(), | ||
function_id: instance_id.function_id.to_string(), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).