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

generate an async interface #6985

Closed
deanorderly opened this issue Apr 28, 2020 · 6 comments
Closed

generate an async interface #6985

deanorderly opened this issue Apr 28, 2020 · 6 comments

Comments

@deanorderly
Copy link

For some reason the generated code is an abstract class like so

public static abstract class AuthApiImplBase implements io.grpc.BindableService {

could we possibly slightly change that line to

public static abstract class AuthApiImplBase implements AuthApi, io.grpc.BindableService

so that there is an interface. This can be very powerful for extensibility. I would like to implement that interface right now on clients and servers and hide some json underneath it while we slowly get to gRPC from json(ie. getting our foot in the door this way at the very least so can swap it out entirely later but unfortunately with no interface here, it has made it a bit harder).

Another thought...java only allows extending from a 'single class' so in the future it would be better that one does not have to extend AuthApiImplBase as well so services could extend another class but I am guessing that would be too huge of a rework at this point.

@ejona86
Copy link
Member

ejona86 commented Apr 29, 2020

We can't have that sort of interface in the generated code, because then if a new method were added to the gRPC service it could break existing code. We purposefully removed the interfaces. See #1469.

We'd like to have interfaces, but it will require dropping Java 7 support, to let us specify default implementations for the methods. Dropping Java 7 is tracked in #4671.

@deanorderly
Copy link
Author

@ejona86 First, you rock! thanks for the help. ok, ohhh, very interesting. I am confused though so can I get some clarity here

If you have

public static abstract class AuthApiImplBase implements AuthApi, io.grpc.BindableService

and 'everyone' extends AuthApiImplBase, then there is no breaks as he has the default methods, right? The interface is for those that WANT to break on purpose in a monobuild fashion IMHO.

At twitter, we sort of had the same issue with thrift so what happened in the end is mock objects who did not want to break in face of adding methods extended AuthApiImplBase. REAL implementers of the service however were required to change 'with' the added method or the monobuild would fail.

Is there a happy medium like this somewhere so we can slip in the interface and all the gRPC code you generate (including the service) extends AuthSerivceImplBase.

of course, it would be nice to have the choice on generating a service that extends AuthServiceImplBase vs. having it extend the interface(which honestly, us small companies would prefer....well, even at twitter, that was the preference actually and that repo was bit).

thoughts?

thanks,
Dean

@deanorderly
Copy link
Author

For more clarity, my grpc-json plugin allows the controller to implement the AuthApi interface OR extend the AuthApiImplBase sooooo, then the developer can decide when the proto file changes, do they

  1. want their service to stop compiling so the developer has to fix it
  2. not break their service and calls to that method will return some error

@ejona86
Copy link
Member

ejona86 commented Apr 29, 2020

The interface is for those that WANT to break on purpose in a monobuild fashion IMHO.

We don't support that, and believe we can't support that because we can't trust our users. Someone would add a method to a service (which they are allowed to do) and it would break someone would would say that API compatibility was broken (which it was) and expect the change to be reverted. FWIW, this isn't just theoretical, some gRPC languages mistakenly had interfaces, and it caused problems. So this has been shown to be a real issue.

Even in a monorepo, causing new methods to existing code is a problem. The main reason it is sorta okay in a monorepo is you tend to only have one implementation (or maybe a second for testing).

I certainly understand the appeal, but there's a reason why we can't have nice things.

At twitter, we sort of had the same issue with thrift so what happened in the end is mock objects who did not want to break in face of adding methods extended AuthApiImplBase.

Right. You've put a social practice into place to manage it. We don't have such leverage on our users and we cannot reasonably expect "the masses" to "play by the rules" or even be aware of the rules. And honestly, we see lots of users that "know the rules" (in other circumstances) and choose not to follow them and then blame us or expect us to bail them out when they have issues. Virtually all of them say something along the lines of "I didn't write the code" or "I can't change the code."

@deanorderly
Copy link
Author

@ejona86 thanks for the valuable context. 'damn customers' ;). Nah, I totally get it. We all strive to create an easy experience for users. Too many developers 'fight' the customer so glad to see how well you guys adapt.

I guess you can close the issue. One more thought is I wonder if grpc could have an extra flag -generateInterface for power users. In this fashion, my dev team's addition to our monorepo break the build if they forget to add it to the service. Your choice though. I'll defer to you having interacted with way more customers than me. Feel free to close or leave open with the modification on adding a flag -generateInterface. I totally get adding one more thing like this over time becomes maintenance hell.

@ejona86
Copy link
Member

ejona86 commented Apr 30, 2020

A flag would be easy from a basic maintenance standpoint. The problem with a flag is that with the generated code "there can only be one." Each .proto sort of needs a canonical .java generated for it. Instead of having a flag to protoc-gen-grpc-java it would probably be better to add an option to the .proto. But having an option still opens us up to "people doing it wrong."

I really wish it were more clear when we will be able to drop Java 7 support. We can spend energy working around Java 7, but things would really be a lot easier if we could rely on Java 8 language features.

@ejona86 ejona86 closed this as completed Apr 30, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants