-
Notifications
You must be signed in to change notification settings - Fork 64
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
Initial implementation of actor runtime #99
Conversation
d3207fb
to
29459bc
Compare
@danielgerlag notice the linter failures |
@NickLarsenNZ can you help review this? |
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.
Thanks for the contribution @danielgerlag, it is exciting to see actors come to the dapr/rust-sdk.
Also, really appreciate the tests given that the rest of the SDK appears to lack them.
I have left a few comments which I hope are not taken as merge-blockers, but rather as conversation starters which might be helpful both in terms of design but also for future contributors.
With the SDK still being alpha, I'm of the opinion that we should be taking in these sorts of contributions even if they need some adjusting down the track. That will keep contributors interested as well as let people try things out early (it will be trickier once this is out of alpha).
meta: @yaron2, I think it could be a good idea for non-trivial decisions to be recorded as LADRs (Lightweight Architecture Decision Records) for two main reasons:
- There is a recorded explanation of why things are they way they are for future contributors.
- To help reviewers skip over things that are inconsequential, and to focus on the important parts (remove blockers, and avoid having to prompt for explanations on each PR).
|
||
1. Create a struct with your custom actor methods that map to [Axum handlers](https://docs.rs/axum/latest/axum/handler/index.html), use [Axum extractors](https://docs.rs/axum/latest/axum/extract/index.html) to access the incoming request and return an [`impl IntoResponse`](https://docs.rs/axum/latest/axum/response/trait.IntoResponse.html). | ||
Use the `DaprJson` extractor to deserialize the request from Json coming from a Dapr sidecar. | ||
```rust |
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.
nitpick: I think the example should also include the struct MyActor {]
and any derive macros necessary.
src/server/actor/mod.rs
Outdated
} | ||
|
||
#[macro_export] | ||
macro_rules! actor { |
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.
I wonder if it might be preferential to provide a derive or proc macro to tightly couple it with the struct?
eg:
#[dapr::actor]
struct MyActor {}
or
#[derive(dapr::actor)]
struct MyActor {}
The main issue I have with the macro_rules
version is how decoupled it is from the struct definition.
- As a code reader, there's nothing on the struct giving hints as to what it is for (doc comments and struct name aside). I would have to look somewhere around it (if I knew what I was looking for).
- As an actor implementor it is not so clear where the macro should be put (eg: inside/outside a function). Of course the compiler will likely make this clear when it is in the wrong place, otherwise the implementor might need to dig into the macro source to get a hint where it goes (documentation aside).
- As an implementor it is easy to forget to mark the trait, whereas when I am defining the struct that is the place where I am already thinking about which macros to put on it.
A lot of this comes from personal taste (as in, not from an authoritative standpoint), so I'm interested in generating some conversation around it to see what others think from a user perspective.
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, I think from a dev experience point of view... this would be better, but I think we need a separate crate to host proc macros. I might need some help with pipelines / publishing to do that.
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.
@NickLarsenNZ I have changed this to use the #[dapr::actor]
attribute macro, like this:
#[dapr::actor]
struct MyActor {}
Not sure if we need changes for publishing the macro crate.
log = "0.4" | ||
serde = { version = "1.0", features = ["derive"] } | ||
serde_json = "1.0" | ||
axum = {version = "0.6.19", features = ["default", "headers"] } |
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.
As I understand it, this will only work with HTTP (hyper underneath), and not gRPC (tonic underneath)?
Will the actors work with for Dapr in gRPC mode?
My assumption here is that it only works in HTTP mode (because of axum, with hyper underneath).
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.
I do see Tonic being used in the client example, so I assume this is a non-issue. Will leave the comment there anyway.
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.
The dapr side car currently does not support invoking actors via grpc as far as I know, so any actor host would have to support http.
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.
The dapr side car currently does not support invoking actors via grpc as far as I know, so any actor host would have to support http.
That's correct
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.
@yaron2 Is it supposed to be upgraded soon ? or has it be ?
mdata = m; | ||
} | ||
|
||
mdata.insert("Content-Type".to_string(), "application/json".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.
Does the dapr runtime always expect JSON or are other serialization formats supported (eg: protobufs)?
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.
The dapr runtime doesn't seem to care, but according to this dapr doc page, JSON is the preferred default serialization - https://docs.dapr.io/developing-applications/sdks/sdk-serialization/
The other language SDKs seem to serialize actor payloads to JSON automatically, but we probably also need a way to override it?
match self { | ||
ActorError::NotRegistered => write!(f, "Actor not registered"), | ||
ActorError::CorruptedState => write!(f, "Actor state corrupted"), | ||
ActorError::MethodNotFound => write!(f, "Method not found"), | ||
ActorError::ActorNotFound => write!(f, "Actor not found"), | ||
ActorError::MethodError(e) => write!(f, "Method error: {}", e), | ||
ActorError::SerializationError() => write!(f, "Serialization error"), | ||
} |
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.
I was going to suggest using thiserror
to keep the error text with the error variants, but it isn't a dependency.
@yaron2, what is preferred going forward in terms of consistency? Or being alpha, are we just looking to get stuff working first?
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 can be a further optimization down the road, yes.
src/server/actor/runtime/mod.rs
Outdated
pub struct ActorTypeRegistration { | ||
name: String, | ||
factory: ActorFactory, | ||
method_registrations: HashMap<String, Box<dyn (FnOnce(Router, Arc<ActorRuntime>) -> Router) + Send + Sync>>, | ||
} |
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 applied across the board, but I think there need to be some doc comments to describe the types and methods.
src/server/actor/tests.rs
Outdated
let app = dapr_server.build_test_router().await; | ||
let server = TestServer::new(app.into_make_service()).unwrap(); | ||
|
||
let invoke_resp = server.put(&format!("/actors/MyActor/{}/method/do_stuff", actor_id)).json(&json!({ "name": "foo" })).await; |
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.
nitpick: the format string could have the variable inside it, like so:
format!("/actors/MyActor/{actor_id}/method/do_stuff")
src/server/http.rs
Outdated
pub struct DaprHttpServer { | ||
actor_runtime: Arc<ActorRuntime>, | ||
} |
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 comment is more for my own understanding to reconcile implementing existing building blocks in this SDK, and implementing the actors building block.
Is this intended to be a new pattern for launching the dapr server?
Eg: there's the existing tonic
way for the AppCallback. Is this the Hyper (via Axum) counterpart?
On that note, is there something special about actors that makes it different to launch than the existing AppCallback way? I haven't had a good look at the latest protos to see what is defined there (yet). I also asked a variation of this further up, so maybe the answer will be up there.
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.
Actors only support callbacks via HTTP today
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, dapr only supports Http for actors... but this could also be a starting point for all other Http callbacks.
|
||
1. Start actor host (expose Http server receiver on port 50051): | ||
```bash | ||
dapr run --app-id actor-host --app-protocol http --app-port 50051 cargo run -- --example actor-server |
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.
It might be worth pointing out that only --app-protocol http
is support (if that is indeed the case).
Is it expected that --app-protocol grpc
might one day be available?
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.
One day, but not today :)
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
aada8b0
to
9ac2fa8
Compare
@yaron2 I do not... where do I see them? |
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
@NickLarsenNZ would appreciate another review when possible. thanks! |
Signed-off-by: Daniel Gerlag <[email protected]>
@NickLarsenNZ can you review please? |
@NickLarsenNZ I implemented your suggestion around the macro... could you please re-review? |
@danielgerlag please resolve the conflicts and we can then merge |
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
@yaron2 I have resolved the conflicts. I have also updated the CI to publish the macros crate, but am unable to verify that it is correct... if someone could help with that? |
Signed-off-by: Daniel Gerlag <[email protected]>
Signed-off-by: Daniel Gerlag <[email protected]>
Merged! Thanks for this great contribution @danielgerlag. Please update our docs in the docs repo to add Rust Actors there where neded. |
@danielgerlag Can you elaborate on what is missing in that implementation to fully use actors in Rust ? |
* actor implementation Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * nits Signed-off-by: Daniel Gerlag <[email protected]> * tests Signed-off-by: Daniel Gerlag <[email protected]> * make client cloneable Signed-off-by: Daniel Gerlag <[email protected]> * logs Signed-off-by: Daniel Gerlag <[email protected]> * logging Signed-off-by: Daniel Gerlag <[email protected]> * async methods Signed-off-by: Daniel Gerlag <[email protected]> * shutdown semantics Signed-off-by: Daniel Gerlag <[email protected]> * clone actor client context Signed-off-by: Daniel Gerlag <[email protected]> * actor implementation Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * move tests Signed-off-by: Daniel Gerlag <[email protected]> * actor factory Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * wip Signed-off-by: Daniel Gerlag <[email protected]> * readme Signed-off-by: Daniel Gerlag <[email protected]> * pr feedback Signed-off-by: Daniel Gerlag <[email protected]> Signed-off-by: Daniel Gerlag <[email protected]> * cargo fmt Signed-off-by: Daniel Gerlag <[email protected]> * cargo clippy --fix Signed-off-by: Daniel Gerlag <[email protected]> * proc macro Signed-off-by: Daniel Gerlag <[email protected]> * dependency shuffle Signed-off-by: Daniel Gerlag <[email protected]> * Update lib.rs Signed-off-by: Daniel Gerlag <[email protected]> * docs Signed-off-by: Daniel Gerlag <[email protected]> * enable decorating type alias Signed-off-by: Daniel Gerlag <[email protected]> * graceful shutdown Signed-off-by: Daniel Gerlag <[email protected]> * merge issues Signed-off-by: Daniel Gerlag <[email protected]> * cargo fmt Signed-off-by: Daniel Gerlag <[email protected]> * update rust version Signed-off-by: Daniel Gerlag <[email protected]> * publish macro crate Signed-off-by: Daniel Gerlag <[email protected]> * dependency issue Signed-off-by: Daniel Gerlag <[email protected]> * clippy warnings Signed-off-by: Daniel Gerlag <[email protected]> --------- Signed-off-by: Daniel Gerlag <[email protected]>
@cscetbon, I don't think there are any missing features in terms of the Dapr actor model & spec. |
Thanks @danielgerlag the title of this PR was confusing. I tried to ping you on discord to get more information but without success. |
@danielgerlag Using your code it doesn't seem we're able to send a message to another actor without recreating manually a new connection to dapr. If I'm not mistaking it's because client is private in struct ActorContextClient and it seems to be the only way to access the invoke_actor method. |
@cscetbon the invoke_actor method would be on the main dapr client. If you want this client available inside your actor, you can pass it into through the factory method with you register the actor, it allows you to define how an actor should be constructed. |
Thanks @danielgerlag, I am able to make it work. However I have to clone the client twice (one to avoid a move and one to make it mutable to call invoke_actor as it requires a mutable instance), any idea how to avoid this ?
|
Actor Example
This example demonstrates the Dapr actor framework. To author an actor,
Create a struc decorated with the
#[dapr::actor]
macro to house your custom actor methods that map to Axum handlers, use Axum extractors to access the incoming request and return animpl IntoResponse
.Use the
DaprJson
extractor to deserialize the request from Json coming from a Dapr sidecar.There are many ways to write your actor method signature, using Axum handlers, but you also have access to the actor instance via
self
. Here is a super simple example:Implement the
Actor
trait. This trait exposes the following methods:on_activate
- Called when an actor is activated on a hoston_deactivate
- Called when an actor is deactivated on a hoston_reminder
- Called when a reminder is recieved from the Dapr sidecaron_timer
- Called when a timer is recieved from the Dapr sidecarAn actor host requires an Http server to recieve callbacks from the Dapr sidecar. The
DaprHttpServer
object implements this functionality and also encapsulates the actor runtime to service any hosted actors. Use theregister_actor
method to register an actor type to be serviced, this method takes anActorTypeRegistration
which specifies