Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal: PubSub Subscription Streaming #52
Proposal: PubSub Subscription Streaming #52
Changes from all commits
37cd0bd
0a27a06
d2a4dc8
232480b
891480b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
TopicEventResponse
refers to a response on a published message am I right in reading this as it being used as a response for the subscription response too? If so seems a bit too tightly coupled in my opinionThere 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.
How's the event ID computed?
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 believe either from the pubsub itself, or a random UUID here.
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.
Isn't that for cloud events only?
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.
We can generate a unique id per message the same way bulk pub/sub does 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.
WebSockets make things a lot more complicated, including things like authz. We support HTTP/2 now so we should just use server-sent events (since we don't need bi-di streaming). It's much simpler to implement and use, and doesn't introduce a lot more dependencies on the 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.
We can't use unidirectional streaming because the client needs to send back information to the server (Dapr), which is the status for each message received. From the auth perspective this wouldn't be different from how an app authenticates to Dapr today (via an access token, and unauthenticated by default) and even in browser to server scenarios a client access token is used.
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 send-back can be done with regular HTTP requests. When using HTTP/2, they use the same TCP socket, so this wouldn't be an issue.
I would recommend avoiding adding another protocol as it does make a lot of things more complicated.
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 auth remains token based auth, but it doesn't have to pass through a header in this case. The initial handshake contains the token as described in the link you posted. This seems like a standard way for client side auth and is also used by the Azure Websockets (PubSub) service, as one example.
Re: web browser, it'd actually be great to make this HTTP path usable from web browsers, where http/2 doesn't have the same support as websockets. This will provide a clearer usage pattern where backend services can connect over gRPC while web services and/or any client that doesn't have gRPC or http/2 support can use websockets.
The only issue we face with websockets is the separate socket issue. If the same port cannot be shared with the existing HTTP server and the Websockets server than this is likely a non-starter.
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.
Correct, but that's part of the application protocol we need to define, and it does add complexity.
Support for HTTP/2 in browser is just as much as WebSockets. In any case, HTTP/2 is a nice-to-have for long-lived connections but not a requirement (using HTTP/1 would be fine, but would require one separate TCP socket).
Separate socket, not port. So the long-lived connection requires its own TCP socket between the app and Dapr (1 TCP socket for the WebSockets long-lived stream, and then each request from the app will have its own). While in the case of HTTP/2 there's multiplexing being used, so there is at most 1 TCP socket being used (the long-lived stream can share the same socket as every request going to Dapr). This does improve efficiency. But it doesn't require a separate port - the same Go server can serve HTTP/1, HTTP/2, and WebSockets (but this only over HTTP/1 and not HTTP/2, per issue above)
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, so if we don't need a separate port and there's a path forward for HTTP/2 support for WebSockets then I'm comfortable using WebSockets HTTP 1.1 to start with
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 issue for adding WebSockets over HTTP/2 was opened in 2021 and looks like it's not being worked on. It seems that it is not a priority (understandably, as WebSockets is not as popular as it once was).
I think enabling a new communication protocol should be something not done lightly, as we'll be on the hook to support that for a very long time. There's nothing inherently wrong with HTTP/2, which is already supported and we have all tooling and test in place.
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 user experience that comes out of having a full-duplex bidi stream with the receive message/reply to server commands encapsulated within WebSockets is preferable to me than what the experience would be like with SSE, moreover the latter being limited to a text format. We can always change to HTTP2/SSE if we're seeing a reason to do so before moving the API to stable
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.
Although WebSocket is not that popular, it is still very useful in full-duplex bidi stream case. I think it is a good start and we can Iterate it based on users' response and so on.
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 am open to looking at the maintainability aspect of Websockets and we could also potentially ping for the interest in the duplex stream from the community.
But this proposal looks good for gRPC as a starter.
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.
Please can I propose that this proposal tracks the Rust-SDK too :)