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

Phase 1 to add the server interceptor #642

Merged
merged 1 commit into from
Apr 18, 2016
Merged

Conversation

iamqizhao
Copy link
Contributor

No description provided.

dsymonds pushed a commit to golang/protobuf that referenced this pull request Apr 18, 2016
…ion.

See grpc/grpc-go#642 for the corresponding grpc-go change.

Signed-off-by: David Symonds <dsymonds@golang.org>
@dsymonds
Copy link
Contributor

The golang/protobuf change has been pushed: golang/protobuf@331aba2

@iamqizhao iamqizhao merged commit dd82865 into grpc:master Apr 18, 2016
ThomasHabets added a commit to ThomasHabets/qpov that referenced this pull request Apr 19, 2016
@spenczar
Copy link

This backwards-incompatible change broke builds for teammates and produced confusing errors. I can't find any explanation for the changes, nor can I see anything in the milestones which would have helped see that this was coming up. Is there anywhere I can subscribe to announcements of these kinds of breaking changes?

@zellyn
Copy link
Contributor

zellyn commented Apr 20, 2016

@spenczar there are version checks in both grpc-go and go-protobuf specifically to ensure that the versions match. If you upgrade one, upgrade the other too.

@iamqizhao
Copy link
Contributor Author

Sorry for the breakage. This breaks only if you cache and use the generated code directly. Otherwise, it should be transparent to users. Anyways, I am going to pay more attention on this and will announce any potential breaking changes on a github issue. You do not need to subscribe anywhere else.

michael-berlin added a commit to michael-berlin/vitess that referenced this pull request Apr 20, 2016
michael-berlin added a commit to michael-berlin/vitess that referenced this pull request Apr 20, 2016
grpc/grpc-go#642.

(We didn't pin the protobuf grpc Go plugin yet and got broken because of
that.)
@riannucci
Copy link

Drive-by/Quick question: Why do the handler methods/types take interface{} instead of proto.Message?

@iamqizhao
Copy link
Contributor Author

@riannucci It is because gRPC and protobuf are not a bundle. Users are allowed to use other things as their IDLs. But all the IDLs need to share this same API -- this interface{} could be a proto.Message or a slice or something else.

@nodirt
Copy link

nodirt commented Apr 20, 2016

Why code generation had to be changed? The interceptor in the generated code seems to be unnecessary because handler func doesn't pass any data to the interceptor that the caller of the handler func doesn't already have. Note that in is passed to dec

@iamqizhao
Copy link
Contributor Author

For the streaming rpc interceptor (which I am working on) the codegen is not changed. For unary rpc, we have to do that because protobuf decoding happens in the generated code. The interceptor impl needs to call handler(ctx, in) by itself to complete the rpc as usual if they want. We will have a doc to describe the design and show some examples.

@nodirt
Copy link

nodirt commented Apr 20, 2016

Yeah, I see. I just tried to write a patch for (*Server).processUnaryRPC and got stuck on the fact that the handler that interceptor receives is a func that accepts a decoded messages whereas generated handler receives a func that does decoding, not a decoded message.

maditya added a commit to YahooArchive/coname that referenced this pull request May 10, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
@lock lock bot unassigned dsymonds Jan 20, 2019
@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants