Experimental download-style streaming responses #110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Use "EncodeMessage" instead of "Marshal" in serve<methName>Protobuf so message length is written properly * Flush stream response after each message if response writer implements http.Flusher * Add streaming "MakeHats" endpoint to example service
* Also, add tests for example server * JSON clients currently fail to stream (rpc call gets EOF err instead of RespStream)
* Add new "test_example" target to Makefile. Example tests are passing for protobuf clients; JSON tests would fail, but are commented out * Return normal twirp errors from download requests that fail before the stream is created * Consolidate generator logic for unary and download rpc types in generateServerProtobufMethod * In the example server, use chan of HatOrError type to send mid-stream errors
…ity with older versions of go
* Define `<Type>OrError` struct that is a union of the `<Type>` and `error` return values from `Next` * Define `New<Type>Stream` constructor that takes a channel of `<Type>OrError` and returns an implementation of the `<Type>Stream` interface
mikeylemmon
force-pushed
the
v6_streams_alpha
branch
from
88bcfbb
to
96a7514
Compare
mikeylemmon
force-pushed
the
v6_streams_alpha
branch
from
96a7514
to
fe19e01
Compare
|
@spenczar latest commit updates to the new The generated server code is still flushing on every message, probably a couple weeks before I'd be able to get into attempting a |
mikeylemmon
force-pushed
the
v6_streams_alpha
branch
from
357ae0d
to
b8fadd5
Compare
|
I've been discussing this branch with @fritzherald over slack. I'm going to merge it right now into a non-master branch, and then start doing some of my own work on top. Thank you so much for all the hard work on this, @fritzherald! Your feedback on the design has really helped make this thing better. You rock. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #, if available: #70, #3
Description of changes:
This PR fleshes out some of the ideas discussed in the streaming-related issues, adding support for download-style streamed responses to the go generator, as well as a streaming
MakeHatsrpc to the example and atest_exampletarget to the root Makefile.Some things to note:
example/cmd/server/main_test.go. Other than the example server tests, these additions don't have test coverage yet.generator.go. In areas of the file where I was already making changes, I consolidated to tabs, resulting in a couple spots in the diff that are just whitespace changes.I'll be posting deeper thoughts/feelings in #70 as that is where the bulk of discussion has been taking place. Looking forward to feedback!
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.