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

Added UnimplementedServer for server interface #785

Merged
merged 14 commits into from
Mar 15, 2019

Conversation

prannayk
Copy link
Contributor

Solves the issues raised in #784 @dfawley

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@prannayk
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@dfawley
Copy link
Contributor

dfawley commented Jan 15, 2019

cc @lyuxuan

protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/testdata/grpc/grpc.pb.go Outdated Show resolved Hide resolved
protoc-gen-go/testdata/grpc/grpc.pb.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Show resolved Hide resolved
@puellanivis
Copy link
Collaborator

It seems the SafeImplementation has been changed to UnimplementedServer, does it make sense to rename the PR title?

protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
@prannayk prannayk changed the title Added SafeImplementation for Server & Client interface Added UnimplementedServer for server interface Jan 20, 2019
@dfawley
Copy link
Contributor

dfawley commented Jan 22, 2019

Also, “…can be embedded…” would sound better? Saying “should” here suggests that the full implications must be understood and carefully weighed before choosing a different course. Meanwhile, this is more of a nice optional feature.

FWIW, I would argue embedding this should be considered a best practice for all services. Otherwise, newly added methods will break your code - but adding methods to proto services is not supposed to be a breaking change.

protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
@prannayk
Copy link
Contributor Author

Also, “…can be embedded…” would sound better? Saying “should” here suggests that the full implications must be understood and carefully weighed before choosing a different course. Meanwhile, this is more of a nice optional feature.

FWIW, I would argue embedding this should be considered a best practice for all services. Otherwise, newly added methods will break your code - but adding methods to proto services is not supposed to be a breaking change.

True, but its an optional feature for now, and gives added functionality to those who would want the said safety. Therefore the said language is better.

protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
@prannayk
Copy link
Contributor Author

prannayk commented Feb 2, 2019

Hey @dfawley @puellanivis does this look good now?

@prannayk
Copy link
Contributor Author

Let me know if any other changes need to be done @dfawley @puellanivis

Copy link
Contributor

@dfawley dfawley 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 to me, with one minor change requested. @dsnet are you OK with merging this after that is addressed?

protoc-gen-go/grpc/grpc.go Outdated Show resolved Hide resolved
@dfawley
Copy link
Contributor

dfawley commented Mar 4, 2019

@dsnet - any concerns with this PR? If not, I think it is ready to be merged.

@dsnet
Copy link
Member

dsnet commented Mar 5, 2019

I'm out of the office till beginning of next week. I can take a look on next Monday.

@dfawley
Copy link
Contributor

dfawley commented Mar 13, 2019

Friendly ping @dsnet. grpc-go was just broken by a method we didn't implement; this PR would have prevented that.

PR fixes

space removed in return line

change errUnimplemented to real function

Fixed PR issues

errUnimplemented changed to real function, PR fixes
@dsnet dsnet merged commit 318d17d into golang:master Mar 15, 2019
@prannayk prannayk deleted the feature#784 branch August 4, 2019 20:40
srenatus added a commit to chef/automate that referenced this pull request Aug 14, 2019
### Release notes

- https://github.com/grpc/grpc-go/releases/tag/v1.23.0
  - much stuff! a bunch of HTTP2-related fixes -- we don't _expose_ go-grpc's listener, but we use it interally all over the place
  - there's `*satus.Is` now: grpc/grpc-go#2868 -- we could make some use of that, I suppose.
  - a bunch of perf improvements, always welcome :)
- https://github.com/golang/protobuf/releases/tag/v1.3.2
  - small fry, but nice stuff: golang/protobuf#785 adds an Unimplemented method to each server, allowing old servers to be used when there's proto changes.

* deps: bump protobuf (1.3.2) and go-grpc (1.23.0)
* deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [revendor]
* deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [regenerate]

Signed-off-by: Stephan Renatus <srenatus@chef.io>
thaJeztah added a commit to thaJeztah/swarmkit that referenced this pull request Oct 20, 2019
full diff: golang/protobuf@v1.2.0...v1.3.2

protobuf v1.3.2:

- golang/protobuf#785: grpc code generation: add an UnimplementedServer type implementing each server interface, returning an unimplemented error for each method
- golang/protobuf#851: convert prints to os.Stderr to use log.Printf
- golang/protobuf#883: jsonpb: fix marshaling of Duration with negative nanoseconds

protobuf v1.3.1:

- The set of dependencies specified in go.mod has now been reduced to only the standard library.

protobuf v1.3.0:

- golang/protobuf#699: add a go.mod module file
- golang/protobuf#701: stop generating package "// import" comment
- golang/protobuf#741: deprecate {Unm,M}arshalMessageSet{JSON}
- golang/protobuf#760: different internal implementation of oneofs
    - `.pb.go` files generated by protoc-gen-go@v1.3.0 will require the proto@v1.3.0 package to work
- various minor changes to code generation

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
gopherbot pushed a commit to protocolbuffers/protobuf-go that referenced this pull request Nov 8, 2019
Also add deprecation comments on methods.

Forward port:
  golang/protobuf#785
  golang/protobuf#952

Fixes golang/protobuf#816

Change-Id: Id4d9f08b39fe16eaf57fb7a92fb8ada222b5cbf4
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/205246
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
@golang golang locked and limited conversation to collaborators Jun 26, 2020
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.

6 participants