Skip to content

Stateless grpc connection#19

Open
shiroyasha wants to merge 6 commits into
masterfrom
stateless-grpc-connection
Open

Stateless grpc connection#19
shiroyasha wants to merge 6 commits into
masterfrom
stateless-grpc-connection

Conversation

@shiroyasha
Copy link
Copy Markdown

Disclaimer: This is still a draft, tests, and code quality are still pretty poor.
There are several errors that still need to be caught.

I'm attaching you to check out the public API of the clients.

edit: I can't attach @mattrym for some reason. I'm pinging you directly.

Copy link
Copy Markdown
Member

@radwo radwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👏

Comment thread lib/util/grpc.ex Outdated
Util.Grpc.Client.new(:billing_service, "localhost:50051", BillingApi.Stub)
])

3. User Grpc.do to communicate with your upstream services:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
3. User Grpc.do to communicate with your upstream services:
3. User Grpc.call to communicate with your upstream services:

Comment thread lib/util/grpc.ex Outdated
2. Sends the RPC request
3. Waits for the result, or times out
"""
@spec rpc(client_name(), GRPC.Channel.t(), function()) ::
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@spec rpc(client_name(), GRPC.Channel.t(), function()) ::
@spec call(client_name(), GRPC.Channel.t(), function()) ::

Comment thread lib/util/grpc.ex
Comment thread lib/util/grpc/state.ex Outdated
end

def find_client(name) do
Agent.get(__MODULE__, fn state -> Map.fetch!(state, name) end)
Copy link
Copy Markdown

@mattrym mattrym Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing: Agent.get/2 callback is executed internally by Agent process, and Map.fetch! raises unhandled exception in case of failure. Calling find_client/1 with unknown name will crash the State, not sure if that's by design?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll have to fix it ✍️

{:ok, result}

{:error, err} ->
inc(rpc, "response.error.count")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes we send:ok response, and an error status is present in the status field. Can we check if this field is there and log accordingly?

@shiroyasha shiroyasha force-pushed the stateless-grpc-connection branch from e8cd47d to a45ab9e Compare April 12, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants