-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement ingress client #42
base: main
Are you sure you want to change the base?
Conversation
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 like the direction! I left few comments here and there, but overall looks good!
Would it be ok to just add the macro code on top of this PR? This way I could have a full understanding of all the moving parts here.
src/ingress/result.rs
Outdated
} | ||
|
||
/// This struct encapsulates the parameters for retrieving a result of an invocation or workflow. | ||
pub struct IngressResult<'a, Res = ()> { |
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.
When reading the code, i've found this name confusing, Result
usually has a very specific meaning in Rust. I think it should be something like AttachInvocationRequest
or something like that.
src/ingress/result.rs
Outdated
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.
file name is super confusing too :)
src/ingress/mod.rs
Outdated
} | ||
|
||
/// Create a new [`IngressResult`]. | ||
pub fn result<Res>(&self, target: ResultTarget) -> IngressResult<Res> { |
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 naming result should be replaced by attach
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 problem here is that the method returns a type that the user will have to either call .attach()
or .output()
on. So calling it attach
will make the chain look a bit weird (e.g. .attach().attach()
).
The actual usage will look something like this (see my first post for more examples):
let res = client
.service_result::<MyServiceResult>()
.my_handler("lorem_ipsum") // <- this is the idempotency key
.attach() // or .output()
.await?;
let res = client
.workflow_result::<MyWorkflowResult>("Me")
.attach() // or .output()
.await?;
The term "result" was taken from this part of the docs:
Restate allows you to retrieve the result of workflows and invocations with an idempotency key.
Do you have any suggestions for a better term? Some ideas: response
, answer
.
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.
ahhhh got it, i think then the name should be workflow_handle
/invocation_handle
, see here https://docs.restate.dev/javadocs/dev/restate/sdk/client/Client
Thanks for the review! |
@patrickariel I'm not sure feature gating what the macro generates is a good idea, it might be counter-intuitive. The way I'm thinking about it is that the macro should always generate a client and link to some "trait" that is always available in Let me think this through though before asking u to make any further changes to this PR... |
Whoops sorry about that, pushed a few commits before I had a chance to see your comment.
This is also something that I've been thinking. Maybe we can copy a pattern often seen in other Rust libraries, where a generic "executor" is passed to the domain client as late as possible. This way the user can simply implement the trait if they want to use their own custom executor. For example, in oauth2: let client = BasicClient::new(github_client_id)
.set_client_secret(github_client_secret)
.set_auth_uri(auth_url)
.set_token_uri(token_url)
.set_redirect_uri(
RedirectUrl::new("http://localhost:8080".to_string()).expect("Invalid redirect URL"),
);
let http_client = reqwest::ClientBuilder::new()
.redirect(reqwest::redirect::Policy::none())
.build()
.expect("Client should build");
let token_res = client.exchange_code(code).request_async(&http_client).await; sqlx also uses a similar pattern: async fn notify(pool: &PgPool, s: &str) -> Result<(), sqlx::Error> {
sqlx::query(
r#"
SELECT pg_notify(chan, payload)
FROM (VALUES ('chan0', $1)) v(chan, payload)
"#,
)
.bind(s)
.execute(pool)
.await?;
Ok(())
} |
An initial implementation for the ingress client (#33). I'll do the macro in a separate PR, once the current implementation is finalized.
A few usage examples:
MyService
MyVirtualObject
MyWorkflow
There was an issue with idempotency keys and virtual objects, but it looks like that it got fixed on
1.2.0
.