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.

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

Send RPC from skinny server #92

Merged
merged 15 commits into from
Oct 20, 2018
Merged

Send RPC from skinny server #92

merged 15 commits into from
Oct 20, 2018

Conversation

iandioch
Copy link
Member

Closes #55.

Get gRPC working on server. Shown here by contacting the greeting card example service.

Copy link

@devoxel devoxel left a comment

Choose a reason for hiding this comment

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

Looks grand, one comment about how you're creating the connection

skinny/main.go Outdated
}
addr := host + ":8000"
return func(w http.ResponseWriter, r *http.Request) {
conn, err := grpc.Dial(addr, grpc.WithInsecure())
Copy link

Choose a reason for hiding this comment

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

In general, we shouldn't be creating grpc connections everytime we open a response, instead we should use an existing connection (via a client), and only make the connection once when the server is started.

we may want an s.greeter that is the client, so it's easy to pass a mocked client in during a test

Copy link
Member Author

Choose a reason for hiding this comment

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

s/response/request?

You're probably right, and I like the idea of s.greeter. That fits the problem I was trying to figure out with passing in a mock in tests. I will change this.

skinny/main.go Outdated Show resolved Hide resolved
@iandioch
Copy link
Member Author

I'd like to reformat the RPC services in the serverWrapper into some common struct or interface, so that there's just something like:

type RPCService struct {
  conn *grpc.ClientConn
  service pb.blahBlahClient
}

in the serverWrapper for each RPC service. This seems cleaner than holding a conn and a client for each individual RPC endpoint individually in the serverWrapper. However, because stuff with generics doesn't really work in Go, it's tough to do this with in a nice way for every blahBlahClient without losing type safety.

I'll hopefully play with this again in future to find a nice way, but in the meantime, PTAL a this PR.

@iandioch iandioch merged commit 1ada854 into master Oct 20, 2018
@iandioch iandioch mentioned this pull request Oct 21, 2018
@iandioch iandioch deleted the n/skinnyrpc branch October 24, 2018 22:58
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.

2 participants