-
Notifications
You must be signed in to change notification settings - Fork 743
Add support for VM gRPC services #3294
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
base: master
Are you sure you want to change the base?
Conversation
56ba07f
to
a67b617
Compare
message HttpResponse { | ||
// header is sent to synchronize any header modifications from the plugin | ||
// process | ||
repeated Element header = 1; |
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.
grpc requires a header to indicate the end of an rpc, which is directly set onto the map in the plugin process (ref). We need to propagate this update back into avalanchego (I believe current rpcchainvm is not handling plugin-side header updates correctly).
e59b451
to
e0a43d2
Compare
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.
Would it be possible to elaborate as to the motivation for this addition? The current 'why this should be merged' in the description seems more like 'what'.
|
||
wg.Wait() | ||
|
||
require.NoError(stream.CloseSend()) |
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.
(No action required) Will the stream be closed automatically on going out of scope (i.e. this explicit close is superfluous)? If not, maybe ginkgo.DeferCleanup
is suggested?
Added |
The upgrade failure is legit - I'm guessing you might need to rebase? From the logs:
|
I think my coreth branch fell behind master. Gonna rebase my coreth branch again. |
This PR has become stale because it has been open for 30 days with no activity. Adding the |
b58e928
to
c585b94
Compare
b81c52b
to
efc3fda
Compare
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Why this should be merged
Adds native support for GRPC services being registered over the avalanchego http server. This adds support for streaming via grpc as an alternative to websockets.
How this works
h2c
's http handler to support plaintext http2Handle
rpc that includes headersrequest.Body
so that the plugin process can stream the body instead of the current behavior which usesio.ReadAll
which may not be possible in the case of a streaming rpc.How this was tested
Added a e2e test for unary + streaming rpcs over rpcchainvm