Skip to content

Actors: bi-directional streaming #72

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JoshVanL
Copy link
Contributor

No description provided.

Comment on lines 107 to 109
Each outbound `RequestRequest` message includes a `uid` field, which is a simple incrementing `uint64`.
This field must start at `1` and increment by `1` for each new request.
If a message is received from Daprd with a `uid` that is not `+1` of the previous message, the stream will be terminated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this from Daprd or from the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the client to daprd. Sorry, that's poorly worded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, that makes sense then. Thanks for clarifying 🙏

Copy link

@WhitWaldo WhitWaldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some way for the runtime to tell the SDK about connection errors at initialization so the client doesn't proceed with setup assuming everything is fine and working.


```proto
message Request {
oneof actor_request_type {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an error channel exposed here somewhere so that if the client requests the creation of an actor and some prerequisite on the runtime isn't met (e.g. cannot connect to the specified actor state store), such a message can be sent back to the SDK so an exception can be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending the error response on initialization should be covered in the ResponseInitial message sent back to the client.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if an error is encountered later on after initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be covered by the ResponeResponse message. I'm not sure whether the error should be a generic error which lives outside the oneofs, or needs to be specific to each oneof API type. I'm assuming it needs to be inside each oneof message so that it can be fully typed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially as this channel is potentially open for a little while, it'd be better to have a oneof in place so the SDK can understand if the error is transient and something that just needs the channel re-opened for or if it's more of a fatal error (and should just be logged and given up on).

I'll have to find the issue, but it's a complaint on one of the recent building blocks that when there's a failure, the SDK doesn't handle it really well because there's no real information from the runtime what to do about it except that something went wrong. It'd be great to correct that scenario here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants