Fixes for Issue 1 and Issue 3 #2
Open
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
Module which deals with records needs to be given as input to unary RPC call. This can be presented as a tuple to the Options list that is passed to grpc_client:unary().
{msgs_as_records,ModuleName}.
And user is expeced to generate records based header file using gpb. This can be out of the scope of grpc_client API.
For example for our route_guide example:
Point = #'Point'{
latitude = 409146138,
longitude = -746188906
},
Return = route_guide_client:'GetFeature'(Connection, Point, [{msgs_as_records,route_guide_pb}]).
API Return will also include a record:
{ok,#{grpc_status => 0,
headers => #{<<":status">> => <<"200">>},
http_status => 200,
result =>
{'Feature',<<"Berkshire Valley Management Area Trail, Jefferson, NJ, USA">>,
{'Point',409146138,-746188906}},
status_message => <<>>,
trailers => #{<<"grpc-status">> => <<"0">>}}}
Open
Details of this request in "Issue 3".
With this code, user can create a streaming RPC connection by specifying how messages have to be deliverd.
{ok, Stream} = grpc_client:new_stream(Connection, 'RouteGuide', 'RouteChat', route_guide, [{async_notification, self()}]).
And when a notification arrives on the stream, a message of the following form {notification,Response} will be delivered to the calling process.
vasu-dasari
changed the title
There was a problem hiding this comment.
Hi @vasu-dasari thanks for this pull request and many apologies for the long delay. I have just a couple of minor comments. Could you please address them and then I can merge.
src/grpc_client.erl
Outdated
| @@ -180,16 +180,14 @@ new_stream(Connection, Service, Rpc, DecoderModule, Options) -> | |||
|
|
|||
| -spec send(Stream::client_stream(), Msg::map()) -> ok. | |||
| %% @doc Send a message from the client to the server. | |||
| send(Stream, Msg) when is_pid(Stream), | |||
| is_map(Msg) -> | |||
| send(Stream, Msg) when is_pid(Stream) -> | |||
There was a problem hiding this comment.
Could you please update the function spec here?
src/grpc_client_stream.erl
Outdated
| @@ -314,6 +318,9 @@ add_metadata(Headers, Metadata) -> | |||
| lists:keystore(K, 1, Acc, {K,V}) | |||
| end, Headers, maps:to_list(Metadata)). | |||
|
|
|||
| info_response(Response, #{async_notification := Client} = Stream) when is_pid(Client) -> | |||
| Client ! {notification,Response}, | |||
There was a problem hiding this comment.
I'm wondering whether it would be better to have the notification message as something like {grpc_notification, Response}. Using the atom notification seems too generic.
Specs to reflect message to be of type tuple (records) or map
As per code review comment, update notification sent to registered clients as {grpc_notification,Response}
|
@cmullaparthi Updated the code as recommended. Let me know if more needs to be done. |
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.
The module which deals with records needs to be given as input to unary RPC call. This can be presented as a tuple to the Options list that is passed to grpc_client:unary().
{msgs_as_records,ModuleName}.
And the user is expected to generate records based header file using gpb. This can be out of the scope of grpc_client API.
For example for our route_guide example:
Return will also be presented to caller as a record: