Use bytes to represent gRPC message body #833
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
tiziano88
force-pushed
the
tzn_grpc_req_bytes
branch
2 times, most recently
from
87630b3 to
129d6d1
Compare
tiziano88
commented
Apr 9, 2020
oak/server/module_invocation.cc
Outdated
| @@ -223,7 +221,7 @@ void ModuleInvocation::BlockingSendResponse() { | |||
| auto callback = new std::function<void(bool)>( | |||
| std::bind(&ModuleInvocation::SendResponse, this, std::placeholders::_1)); | |||
| stream_.Write(bb, options, callback); | |||
| } else if (!grpc_response.has_rsp_msg()) { | |||
| } else if (grpc_response.rsp_msg().empty()) { | |||
There was a problem hiding this comment.
This is causing tests to fail now, I am guessing because an empty bytes field is indistinguishable from a default message. @daviddrysdale do we need to keep this distinction between last message with and without payload?
There was a problem hiding this comment.
I fixed it by assuming that only exactly one between rsp_msg and status is set. @ipetr0v is this compatible with the Rust implementation too? If so, should we make that a oneof, and / or explain in a comment in the proto message?
ipetr0v
approved these changes
Apr 9, 2020
daviddrysdale
approved these changes
Apr 9, 2020
| @@ -29,12 +28,12 @@ import "third_party/google/rpc/status.proto"; | |||
|
|
|||
| message GrpcRequest { | |||
| string method_name = 1; | |||
| google.protobuf.Any req_msg = 2; | |||
| bytes req_msg = 2; | |||
There was a problem hiding this comment.
Need a comment to explain the expected contents of the field:
// Request message as a serialized protobuf; the message type is deduced from the request's method name.
| bool last = 3; | ||
| } | ||
|
|
||
| message GrpcResponse { | ||
| google.protobuf.Any rsp_msg = 1; | ||
| bytes rsp_msg = 1; |
There was a problem hiding this comment.
Need a comment as above
tiziano88
force-pushed
the
tzn_grpc_req_bytes
branch
from
129d6d1 to
a49c05f
Compare
This will also be useful for project-oak#819
tiziano88
force-pushed
the
tzn_grpc_req_bytes
branch
from
a49c05f to
34df463
Compare
daviddrysdale
added a commit
that referenced
this pull request
May 26, 2020
Changes required by commit 68fbbe3 ("Use Rust Loader for running examples (#993)"): - Add and use a config that doesn't include a translator node. - Add C++ code that is equivalent to oak::grpc::server::init(). Change required by commit 64d3f90 ("Use bytes to represent gRPC message body (#833)"): - Re-work hand-coded response message for current GrpcResponse proto message definition. For #1009
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.
This will also be useful for #819