Use InvocationClient in admin API, introduce new API endpoints for cancel/kill/purge #3227
Conversation
|
A followup of this one is to use this new endpoints in the e2e tests, to make sure they just work fine. |
slinkydeveloper
force-pushed
the
issues/admin-api-use-invocation-client
branch
from
19af187 to
db0a50c
Compare
slinkydeveloper
force-pushed
the
issues/admin-api-use-invocation-client
branch
from
db0a50c to
7a1447c
Compare
slinkydeveloper
force-pushed
the
issues/admin-api-use-invocation-client
branch
from
7a1447c to
c32a935
Compare
There was a problem hiding this comment.
Thanks a lot for creating this PR @slinkydeveloper. The changes look good to me. I left a few minor comments. The main one being whether the outbox messages really need to carry the request id. Apart from this +1 for merging.
crates/partition-store/proto/dev/restate/storage/v1/domain.proto
Outdated
Show resolved
Hide resolved
crates/types/src/invocation/mod.rs
Outdated
| #[derive(Debug, PartialEq, Eq, Clone, Hash, serde::Serialize, serde::Deserialize)] | ||
| pub enum InvocationMutationResponseSink { | ||
| /// The invocation has been generated by a request received at an ingress, and the client is expecting a response back. | ||
| Ingress { |
There was a problem hiding this comment.
What other sinks are we planning to support?
There was a problem hiding this comment.
Just Ingress for now, but it makes sense to have a union type here for future proofing
There was a problem hiding this comment.
It might be one day some of these features we expose them in the context API
| AlreadyCompleted, | ||
| } | ||
|
|
||
| impl From<CancelInvocationRpcResponse> for CancelInvocationResponse { |
There was a problem hiding this comment.
What's the difference between CancelInvocationRpcResponse and CancelInvocationReponse and why is it beneficial to have these two types? Is it possible to have a single type?
There was a problem hiding this comment.
No, I want to have split types to make sure I can model the API without being constrained by however the network serialization works.
| @@ -116,3 +118,145 @@ pub async fn delete_invocation<V, IC>( | |||
| Ok(StatusCode::ACCEPTED) | |||
| } | |||
| } | |||
|
|
|||
| generate_meta_api_error!(KillInvocationError: [InvocationNotFoundError, InvocationClientError, InvalidFieldError, InvocationWasAlreadyCompletedError]); | |||
There was a problem hiding this comment.
I am not sure whether we are optimizing here for the reader or the writer of the code.
There was a problem hiding this comment.
I'm optimizing a structured description of errors. I wished to do this with a macro on an enum TBH, like
enum KillError {
#[error("error msg")]
#[status_code(123, "openapi desc")]
Variant()
}But that ain't easy without writing proc macros 😢, I didn't want to get on that for now. I documented the macro, and there's enough examples how to use, so should be good.
slinkydeveloper
force-pushed
the
issues/admin-api-use-invocation-client
branch
from
c32a935 to
a3fad33
Compare
…/<id>/purge Admin APIs. Add cancel/kill/purge invocation to InvocationClient. These new APIs replace the old delete_invocation single endpoint, and have the new semantics returning 404 when the invocation is not found, etc.
slinkydeveloper
force-pushed
the
issues/admin-api-use-invocation-client
branch
from
a3fad33 to
96f77eb
Compare
This PR does two things:
InvocationClientin admin API