-
Notifications
You must be signed in to change notification settings - Fork 2k
Streaming for communication between sidecars and for (HTTP) service invocation #4903
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
Conversation
Please make sure that this supports load balancing, otherwise we cannot accept this PR. Meaning, if you call between app 1 and app 2 through Dapr and another instance pops up for app 2, it will start receiving requests. |
Also, for the scenario mentioned of transferring a file between two sidecars, why can't users do that with gRPC streaming with Dapr? As Dapr supports streaming naturally all the way through from app to sidecar, sidecar to remote sidecar and then to the target app. |
Yes, this should have no issue with load balancing. It creates a new stream for each invocation, so it can load balance. Internally, at the wire level, it behaves the same as an unary invocation.
This is based on HTTP which supports streaming natively as a protocol, we just didn’t support it in Dapr. HTTP is simpler and is easier to integrate with existing services. In fact, I started thinking about this problem about 2 months ago when I made an attempt at developing a demo using Imaginary (which exposes a HTTP endpoint) through Dapr, and noticed these issues. One other thing to note is that this only supports a scenario where there’s a single request followed by a single response. The goal is to provide a stream-based transport for service invocation, transparently. End-users should continue to think of this as regular service invocation, and not think of this as a stream. gRPC proxying remains the best solution when users need long-running streams and the ability to have two-way, alternating communication. Lastly, as I wrote, this starts with HTTP but it also lays the foundation for supporting streams throughout the runtime. My goal is to support streams for other things including state stores (see #4804) and bindings. |
Alternatively, you can keep existing mechanisms untouched while making streaming explicit and respecting HTTP semantics by introducing the |
I don’t think we need |
Ok, just please make sure to validate that either manually or with a test (better). See my comment above about leaving current mechanism untouched and enabling streaming explicitly using a header. |
Sounds good if there's no risk of behavioral breaking change like load balancing. Once this PR compiles I'll help verify. |
Codecov Report
@@ Coverage Diff @@
## master #4903 +/- ##
==========================================
- Coverage 65.66% 64.62% -1.04%
==========================================
Files 124 127 +3
Lines 12991 13547 +556
==========================================
+ Hits 8530 8755 +225
- Misses 3858 4151 +293
- Partials 603 641 +38
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
66b90e5
to
61bb2e8
Compare
Spin-off from dapr#4903 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Note that I've not abandoned this PR. I just realized this is very big, and a lot of things won't work well until #4979 is merged. I'm the process of splitting it into multiple PRs, also to make it easier to review. |
Spin-off from dapr#4903 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Spin-off from dapr#4903 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
spin-off from dapr#4903 Co-authored-by: Hal Spang <halspang@microsoft.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
7f4df0a
to
10bdc44
Compare
Since #4979 will only be merge in 1.10 the soonest, doesn't it make sense to close this PR for now as this is at least 4/5 months away? |
The “core” PR I’m working on does not involve the API server (app-to-dapr). It only implements partial streaming support by changing the messaging APIs (dapr-to-dapr) and the HTTP channel (dapr-to-app). That’s not full support for streaming but at least implements the core functionality and the required APIs to unblock things like blob stores and cryptography, who depend on core streaming support. Updating the HTTP API server will indeed depend on #4979. I will close this PR once the “core” one is ready, hopefully by end of next week. |
* resiliency.PolicyDefined returns the policy object spin-off from #4903 Co-authored-by: Hal Spang <halspang@microsoft.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> * Tweaks Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Hal Spang <halspang@microsoft.com>
* Improvements to E2E test apps Spin-off from #4903 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> * Fixed error after merge Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> * Fixed panic Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
5106cde
to
4cedda5
Compare
…nvocation Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
4cedda5
to
c597328
Compare
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
This is a slightly reduced version of dapr#4903 that implements streaming only in: - Communications between dapr sidecars (via the gRPC local channel) - HTTP app channel (from Dapr to apps) - Responses from direct messages and actor invocations over HTTP APIs It does not add full streaming support to service invocation, as a few things are missing: - Nothing is changing on gRPC APIs for now, which lack streaming support - HTTP API server does not read request bodies as streams, due to buggy support in fasthttp. This will require removing fasthttp per dapr#4979 Related issues: dapr#3103 (partial fix) and also dapr#4866 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Closing this in favor of #5170 |
Description
This PR is the first step in enabling support for streaming data that transits through Dapr runtimes, with the initial focus on sidecar-to-sidecar communication, as well as service invocation(currently limited to HTTP only).
To explain the issue, here's a demo of a service "client" calling into "server" through Dapr, making an HTTP request. The payload is "big" (about 70KB) and for demonstration purposes, both "client" and "server" are slow at producing or consuming the data.
before.mp4
As you can see, the flow is:
The flow is then repeated exactly the same way (but with roles inverted) for the response.
Because in every step the sidecars read the entire message in memory, this can make it challenging to use Dapr service-to-service invocation with large payloads (for example, images) because it adds significant latency. Additionally, because all the data is kept in memory in both serialized and unserialized forms, there's an increase in memory usage.
This PR changes the behavior, and now service-to-service invocation (same example) behaves this way:
after.mp4
By leveraging streaming, this is much faster, especially when transferring large files or when the producer is slow. Especially the TTFB (Time To First Byte) is much shorter.
What this PR includes, at a higher level:
ServiceInvocation.CallLocalStream
is a variant ofServiceInvocation.CallLocal
that uses streams rather than making unary calls. Internally this uses a buffer of up to 4KB, so messages that are smaller than that (and that are received in a single chunk by the runtime) are transferred in a single block. This should allow having no performance impact for small payloads (compared to unary RPCs).It also includes a lot of internal changes to support the two things above.
For clients that invoke Dapr via regular HTTP calls and receive responses through a regular HTTP server, no change will be needed. They will be able to enjoy the benefits of streaming for free and without doing anything!
For clients that use the Dapr SDKs, we may need to update the SDKs to leverage streaming when possible and appropriate. That is a separate work item.
Lastly, this PR is not "complete" as it doesn't add support for streams everywhere where we could want them. For example, we will want to support clients that use gRPC too. We will also want to leverage streaming for reading/writing into state stores and bindings.
Talking to @artursouza, he recommended focusing on HTTP service invocation first, and take this as an opportunity to lay the groundwork that will ultimately be useful in supporting all other scenarios. In this regard, this PR does provide some solid foundations for the remaining work.
Implementation details
This PR creates a new gRPC method that is used for sidecar-to-sidecar communication.
Currently, sidecar-to-sidecar communication uses the
CallLocal
RPC:This RPC is not removed and is retained for backwards compatibility (e.g. when Dapr 1.8 invokes Dapr vNext), however it is not used otherwise.
A new RPC is added:
The request and response objects are defined as:
In the protos above,
InternalInvokeRequest
andInternalInvokeResponse
are the existing protos, that are used by the unaryCallLocal
RPC. In the streamed version, they are wrapped in a proto together with aStreamPayload
object.How it works:
CallLocalStream
on the target sidecar and establishes the gRPC connection and the streaming RPC.Each chunk is a message of type
InternalInvokeRequestStream
orInternalInvokeResponseStream
sent over the RPC, where:InternalInvokeRequest
orInternalInvokeResponse
with the required keys present.payload
, but that is not required.payload
and MUST NOT contain any other property.payload
withcomplete
set totrue
. That message is assumed to be the last one from the sender and no more messages are to be sent in the stream after that.The amount of data contained in
payload.data
is variable and it's up to the discretion of the sender. In this PR, chunks are at most 4KB in size, although senders may send smaller chunks if they wish. Receivers must not assume that messages will contain any specific number of bytes in the payload.Note that it's possible for senders to send a single message in the stream. If the data is small and could fit in a single chunk, senders MAY choose to include a
payload
withcomplete=true
in the first message. In this case, the entire message contains both anInternalInvokeRequest
/InternalInvokeResponse
with the required keys present AND apayload
with data (if any) andcomplete=true
. Receivers should assume that single message to be the entire communication from the sender.To do
Issue reference
Fixes #3103
Also impacts other issues such as #4866
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: