Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Multiplex gRPC servers #309

Merged
merged 2 commits into from
May 10, 2021
Merged

Multiplex gRPC servers #309

merged 2 commits into from
May 10, 2021

Conversation

serras
Copy link
Collaborator

@serras serras commented Apr 30, 2021

Closes #244

@serras serras requested a review from kutyel as a code owner April 30, 2021 15:04
@serras serras mentioned this pull request Apr 30, 2021
Closed
@akshaymankar
Copy link
Contributor

For my use case at least, it would be more ergonomic if ToMultipleServers was also defined for a type level list as I want to be able to merge arbitrarily many servers.
Also, is it ok if MultipleServers and ToMultipleServers are exposed?

@akshaymankar
Copy link
Contributor

On closer inspection, MultipleServers is the "type level list" I wanted to make. What do you think about exposing that and instead of relying on ToMutipleServers type class? The user of the library can be expected to build MultipleServers using the constructors MSEnd and MSOneMore.

kutyel
kutyel reviewed May 3, 2021
View reviewed changes
grpc/server/src/Mu/GRpc/Server.hs Outdated
@@ -29,6 +29,7 @@ module Mu.GRpc.Server
, runGRpcAppTLS, TLSSettings
-- * Convert a 'Server' into a WAI application
, gRpcApp, gRpcAppTrans
, gRpcMultipleApp, gRpcMultipleAppTrans
Copy link
Member

Choose a reason for hiding this comment

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

As per @akshaymankar suggestion, we should consider exporting MultipleServers too?

Suggested change
, gRpcMultipleApp, gRpcMultipleAppTrans
, gRpcMultipleApp, gRpcMultipleAppTrans, MultipleServers(..)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also gRpcMultipleServerHandlers. Also, if that is being exposed it might be best to remove ToMultipleServers and instead provide functions to just translate tuples into MultipleServers and make these functions directly depend on MultipleServers

kutyel
kutyel previously approved these changes May 3, 2021
View reviewed changes
@serras
Copy link
Collaborator Author

serras commented May 10, 2021

I've pushed a simplification of the approach: instead of using a hand-rolled heterogeneus list (MultipleServers), we can simply wrap each element using a GADT. As a result, the API uses a normal list, which I find much easier to handle:

gRpcMultipleApp msgAvro [Srv server1, Srv server2]

@akshaymankar @kutyel thoughts? If that seems OK, I'll add some documentation and merge the PR.

kutyel
kutyel approved these changes May 10, 2021
View reviewed changes
Copy link
Member

@kutyel kutyel left a comment

Choose a reason for hiding this comment

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

LGTM!

@akshaymankar
Copy link
Contributor

Looks good to me too 💯

@serras serras merged commit d3459db into master May 10, 2021
@serras serras deleted the multiplex-grpc branch May 10, 2021 19:20
kutyel pushed a commit that referenced this pull request Jan 4, 2022
@serras
Multiplex gRPC servers (#309)
23677d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaymankar akshaymankar akshaymankar left review comments

@kutyel kutyel kutyel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiplexing many GRPC servers on same port
3 participants
@serras @akshaymankar @kutyel