-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Support OSGi Remote Services by enhancing grcp protobuf plugin code generation #6981
Comments
We do not have much expertise on OSGi Remote Services and the primary goal of gRPC is to work with its own ecosystems. But if it's something not too far, we could try to make it happen. I am still confused with the functionality of the extract layer of interface abstraction. It doesn't seem to add much value. I could understand if that is something required by the framework. But also why the service interface can only have definitions for non-streaming RPCs? It forces service implementors to put concrete implementations (e.g. no-op) for them even if a unary definition is not meaningful for them. |
As a member of both the OSGi and grcp ecosystems I understand, but this does present an opportunity for grpc to be used within the OSGi specifications (in this case Remote Services). ...i.e. simply by enhancing grpc to support OSGi services.
Having the service interface separate from the impl is required by OSGi services (and Remote Services since RS is an extension of OSGi services). Here's the OSGi services description from spec: https://osgi.org/specification/osgi.core/7.0.0/framework.service.html If desired, I would be happy to point you at other architectural and/or technical description of OSGi services in the spec and/or elsewhere.
Currently OSGi services (nor remote services) include any notion of streaming method calls. That's the primary reason why I excluded them in this first go-around. It may be possible to include them...or even propose an enhancement of remote service spec in the direction of streaming rpcs. Also...one thing that I was thinking about was this: perhaps there should be a service option that allows enables the generation of osgi service interface and abstract impl class. For those that didn't use the option grcp would behave exactly the same as without...but for those who do the service interface and abstract impl generation would be done. |
It sounds like this should be a separate code generator, as it would only be used with OSGi and has the limitations of OSGi Remote Services. It should be relatively easy (not trivial, but not a major undertaking) to make it either using an annotation processor reading the annotations in the grpc generated code or by having another protoc plugin. You may consider https://github.com/salesforce/reactive-grpc/ as a reference. I'll note that since gRPC still supports Java 7, we can't have interfaces for services as adding a new method to a grpc service could break existing code. We purposefully removed such things (#1469) |
Here are my reasons for adding it as a feature for the grpc code generator:
https://github.com/ECF/grpc-RemoteServicesProvider/tree/master/examples health.api project in particular...java code was entirely created via grcp generator plus these suggested changes to generate the service interface class...HealthCheckIntf and the abstract impl of that interface...i.e. AbstractHealthCheckImpl. Summary: Why duplicate what the grpc-java generator already does? A separate generator would have to duplicate most of grpc does.
Summary: Deploying (for multiple platforms), supporting, maintaining a new generator isn't available to me and unless someone has the resources to develop a new 'grcposgi' generator from within grcp it doesn't seem practical.
I don't have the resources to develop this currently, and like I imply above...the community functions around such an undertaking (support, maintenance, etc) aren't available to me. I'm not salesforce, unfortunately.
My comment about discussion on #1469: For OSGi services, clients always access the service through a proxy (created/provided by the distribution provider as part of service import) that implements the service interface. I don't know the whole hierarchy of grcp-generated classes but I think this aspect of OSGi services simplifies the situation wrt adding methods via grcp generation. WRT grcp's generator and interface generation: OSGi has classloader-per bundle and semantic versioning built in...meaning that there can be more than one version of a service interface in a given runtime. It seems to me that in this (OSGi) case that breaking existing servers (not clients) by adding a method in a new version of the service interface is exactly appropriate, and servers can/should target their implementations to one specific version of the service interface. Which brings me to my suggestion: Instead of doing the service interface generation and abstractimpl generation for every grpc method in proto (as my pr does), why not qualify the service interface and abstractimpl generation with a service option where the default is to not generate service interface and abstractimpl? e.g. an osgi service option like: service HealthCheck { I thought about putting something like this in the first pr, but need some more expertise on introducing service options in the generator code and so didn't do it yet. But as I understand it this would make my proposed enhancement completely invisible to those that weren't using grpc-java to create OSGi services. |
We understand your concern and we also believe having interfaces for services is what many other users happy to see. However, as mentioned above we cannot do it for Java 7 (it's a huge breakage for existing users). This is the biggest concern. That's why we suggest implementing a separate codegen. If you have other idea/workaround to avoid this, we are happy to discuss. |
To be clear, grpc's generator does not generate *Proto classes. gRPC users use two generators today, the Java message generator as part of protobuf/protoc and the grpc service generator as part of protoc-gen-grpc-java.
It shouldn't need to. The new APIs you are generating would be off-to-the-side even in the current generator. Your generator would be in addition to the existing generators. I took a look at these classes and it does seem like it would be easy to generate them separately. Basically your generator would just output these two files:
That doesn't mean it is the current gRPC contributors obligation to spend resources on it. But it seriously is not a lot of code. It is work to learn, but not much code at the end. If you look at the contributors to that project, one person did the majority of the work (especially early when things were initially created and plumbed together).
Most of the work is already there and would naturally be reused. We maintain protobuf-gradle-plugin and interact with the maintainers of the Maven plugin as needed. reactive-grpc already has glue for executing a Java plugin instead of a C++ binary (and if more things are necessary there, we would be happy to accept improvements). These generators are deployed on Maven Central like everything else. The work to "deploy, promote, and support a separate generator" is a minor amount of effort above "deploy, promote, and support" grpc-RemoteServicesProvider itself.
That really is ignoring the point that we can't have the interface. We don't get to dictate how users use the interface and some users will misuse it. See #6985 where we recently had the same discussion. |
Separate from the below points: I don't really understand the need to maintain for Java 7 at this point.
My proposal would be that interface generation require a new file or service option whose default is 'false'...i.e. the code generator does exactly as it does now (generates no interface) unless this new option is present. This would allow backward compatibility...i.e. no interface generated unless option to do so is specifically set, and would support both my needs and it sounds like lot of other's needs for those that don't need or want to support java 7. Why wouldn't an option allow for serving both needs? |
I think java 7 is a an android thing @scottslewis and phones are just out there not going away. @ejona86 this thread rocks!! Thanks @scottslewis I am thinking about this SAME problem actually. I might do work there eventually and I think there is a lot of good info here on re-using the protobuf generator and creating a new one. For instance, I create an interface where I use CompletableFuture's in webpieces webserver as we wanted to make changes to *.proto file and controller implementing the interface and not have to restart our development server for rapid development. Anyways, thanks for the link @ejona86 . I have much to research here and the other thread. |
The discussion about dropping Java 7 support has been there for a while. It's been slowing down without too much effort pushing it forward. #4671 is the tracking issue for it. Definitely, there are concerns/blockers need to be resolved before we can make this step.
IMO, this sounds to be OK. But we would need some team-wise discussion. Also, as I mentioned above, I still have concern about the interface has only unary API even if the the service definition is streaming. This is inconsistent to what it should be. As you said, the OSGi remote service currently does not have notion for streaming. Would it be possible to make that happen first? |
"My proposal would be that interface generation require a new file or service option whose default is 'false' lol @scottslewis . no wonder @ejona86 referred me when I said 'the exact same thing' in this thread #6985. lol. Well, I am going to hack this weekend I hope and then I will be back with a bit more insight or questions. |
Neither do we 😄. We certainly see people concerned about it. It seemed some quiet tears were shed when we dropped Java 6 support in 2018 (#3961). But it is unclear how many people have these concerns and how "real" of an issue it is. It's the sort of thing we're mostly blind from, until we take it away and then 2 years later people complain bloody murder. I had hoped that the reduced support from Oracle for Java 7 would have been a strong argument, but not everyone bought that. Blind leading the blind, really. @deanorderly, we're actually fairly comfortable with Android at this point. Newer Java language features work pretty well on Android. Newer Java library features (e.g., suppressed exceptions) are a problem still. We can work around library limitations, but language limitations are part of our API so no such luck.
I completely agree that adding new interfaces in backward compatible. The concern is what happens when those interfaces change. So the option reduces the number of people impacted, but it doesn't stop it from being an API compatibility problem. There is a possibility that these generated interfaces would provide Java 8 defaults, thereby fixing the API compatibility issue. But that would still leave other issues like the new interfaces "don't support streaming." I think my biggest argument is we knew that different people will have different needs for the generated code. Java is a very fragmented language for this sort of thing. But we think we've made it pretty easy for others to create alternative APIs and generated code, and we're quite happy and content with that. We've made it where "you don't have to ask permission" to extend our API and do neat things for your users, as your code generator can do what's best for your users. |
[Scott] Yes, I know...but the point is valie if limited to just *Grpc.
[Scott] remote service developers are then running two generators? I'm trying to make the use of grcp to create, implement, and use osgi remote services as easy as possible, and having a complicated tool chain doesn't do that very well. > > > I don't have the resources to develop this currently, and like I imply above...the community functions around such an undertaking (support, maintenance, etc) aren't available to me. I'm not salesforce, unfortunately. > > That doesn't mean it is the current gRPC contributors obligation to spend resources on it.No, but it also doesn't mean it makes sense to reject a contribution that could be immediately taken advantage by both the grcp community and sizeable osgi community. It seems that defeats a big part of the purpose of having a community...i.e. sharing resources on common problems.
The amount of work to code does not concern me nearly as much as getting the benefits pointed to below of having it be distributed, supported, and maintained by grpc community. I'm willing to contribute to that support and maintenance, of course, if desired.
[Scott] I don't get this point. If they choose to provide the option and are using java 7, then they are choosing to 'misuse it'....just like if they introduce a service interface by typing it into the same package and then use java 7...which of course they can do. If they use the option, comments/warnings could specifically be added for Java 7 users and I'm happy to add that sort of thing.
Seems to me it shows that a fair number of people want something like interface generation...precisely because it's valuable for separating remote service contracts from impl and distribution. Count me and others in that group. If android is the source of the need to permanently support Java 7...why not just prevent using the proposed service option for android in the generator? |
[Scott] I'm happy to be involved in creating a new generator if others already have the necessary expertise and are willing to contribute. I just don't think that a new/separate generator is going to be sustainable for me as primary contributor anytime soon. And sustainability is a self-imposed requirement for me that will likely also be very important to the osgi community (which is relatively spec-based). My own impression is that the need for interface generation for grcp is increasing and need to support Java 7 is waning in this community...but that's my impression. |
[Scott] Practically speaking and with due respect, I don't think 'you can do anything you want' is as empowering as any of us would like. Even though the creation/development of a tool (like a new generator) is not technically difficult given a open API (which I'm thankful for btw), IMHO that's often not the 'hard part' about getting tooling adoption. |
@ejona86 How do I plugin? I have been googling around. Is there docs? I also want to plugin to modify the protobuf as I ask here https://stackoverflow.com/questions/61542792/how-to-plugin-into-grpc-java-to-modify-code-generation-and-add-setnameorclearnu (I can post the answer to SO....I use my questions as a personal knowledgebase so I frequently answer my own questions or you can). I want to wire into gRPC for interfaces and into protobuf for setNameOrClear methods as well. thanks much! |
We have gotten repeated requests for custom codegen for random frameworks. If we accepted them all we'd have probably about 10 APIs now, and we'd have to maintain those APIs approximately forever. We need integrations between gRPC and "arbitrary system X" to be pluggable and separate, so they can live and die separately from "core" gRPC. I'm going to close this because the conversation no longer seems productive. Once we drop Java 7, we'll be happy to have an interface, but such interfaces would include streaming support. Even if this was part of gRPC directly, we would strongly consider it being a separate plugin. |
Characterizing OSGI or OSGi services as a 'random framework' is not even remotely accurate. Please see https://www.osgi.org. I can't speak to the other requests, but seems to me that lots of requests for similar/same thing (i.e. support for interface generation in this case) would have a little more weight with a putative 'open project'.
This is a spurious argument IMO. This seems more a matter of not wanting to support something that your community obviously wants...or rather not wanting to allow the community to support it.
This isn't 'arbitrary system X'. It would be more accurate to call gRPC 'arbitrary system X'.
Only no longer productive because you have apparently tired of listening. I would suggest listening, discussing, and responding to your 'community' more. I certainly expected more. |
I was including Spring in that. And I'd include J2EE if it were relevant to this discussion. It's not that nobody uses these things, it's just that the ecosystem is so fragmented there are lots of heavily used frameworks and there are very strong cliffs between them. And even if you support 5 of them you are still looking at <50% of the ecosystem. They're also easily things we don't use and don't have experience in. As in, we aren't well qualified to maintain such integrations, answer questions, and provide support and if we wanted to be we'd need to divert a significant set of resources and would need to continue spending effort "keeping up with the times." Right now you are the only person that has mentioned OSGi Remote Services to us. Even if OSGi Remote Services are the best thing since sliced bread, that still doesn't mean that the overlap in user populations of OSGi Remote Services and gRPC users would be high. It seems OSGi Remote Services users potentially wouldn't care what protocol was used. The "community" does not exist in just this one repo. It is you as well, and if you make the plugin and maintain the repo then "the community" has access to OSGi Remote Services with gRPC. But realize we don't even maintain a Kubernetes NameResolver plugin in this repo, and that is a lot more front-and-center in our day-to-day lives than OSGi.
We don't have infinite resources, so yes, we totally don't support some things the community obviously wants. Retries for example are still not ready. But it is not at all true that we don't want to allow the community to support it. I've provided methods that it can be done without our involvement, and can be managed by one person. You found the costs of that to be too high, but it is still an available option. Just because we really want something doesn't mean the cost will be to our liking. I've continued the discussion because I hope to convince you I do care that you are enabled to be successful. But I also hope that you understand that I don't have the resources to dedicate to your project. |
I'm (really) not trying to compare relevance/importance/goodness/popularity of frameworks. Further, I'm not asking for anything specific to OSGi Remote Services, rather just a single service interface class. As everyone is no doubt aware, an interface-impl separation is common pattern in java frameworks, including J2EE and Spring etc...as well as OSGI services. Since declaring a service interface is a relatively common need for today's frameworks...for others as well as OSGi services...that's why it occurred to me to contribute an enhancement to grpc-java generator rather than to force OSGi Service Developers to create a service interface in same package 'by hand'...since to use grpc-java they've already declared a service in the proto file.
I understand and appreciate what you are saying about keeping up with the times and the burden involved...and don't want to burden you or your community. However, I think that service interface generation would get both usage and community support (from more than me or just the OSGi community) because: a) as per above, the notion of a service interface as a contract separate from impl is very common across many java frameworks...and you've had requests for it before. b) I'm happy to continue to add to this contribution by providing docs, test code, examples (OSGi and just java), and provide support as much as I can. But I truly don't believe I would be the only one consuming or supporting.
My request of grpc-java is not to generate OSGi remote services. It's only to generate a service interface for the protobuf-declared service. As an example, here's an OSGi bundle where I used this proposed contribution to generate the HealthCheckIntf class (along with HealthCheckGrpc, etc): (see in src/main/etc for classes) This bundle has a manifest with OSGi meta-data in addition to the grpc-generated code. All that my propose contribution did was generate the HealthCheckIntf and AbstractHealthCheckImpl classes. The generated classes and manifest.mf are sufficient to define a complete api bundle for this service. There is more to a remote service than that: additional bundles, the RSA impl bundles, distribution provider based upon grpc bundles, etc). Generating or creating any of that other OSGi-specific stuff is not expected nor desired from grpc.
That's true. That's why ECF's impl of OSGi Remote Services supports a number of different distribution providers, that use different protocols https://wiki.eclipse.org/Distribution_Providers This is a feature of OSGi Remote Services: the service contract/interface is not bound to any transport or distribution system at compile time. > > We don't have infinite resources, so yes, we totally don't support some things the community obviously wants. Retries for example are _still_ not ready.I understand limited resources. I'm not trying to dump a support load on you or your community. I just think that the total support needed for a small enhancement to the generator to generate service interfaces (optionally) would be lower than a new/separate tool. > > I've continued the discussion because I hope to convince you I do care that you are enabled to be successful. But I also hope that you understand that I don't have the resources to dedicate to your project.I am convinced that you care. Thank you for that. I'm not asking for you or this community to dedicate resources to my project. Just to be clear, my project is a spec-compliant impl of OSGi Remote Services...known as ECF. I think it would benefit both communities if OSGi Remote Services could be implemented with grcp distribution: remote service devs able to easily use grcp for distribution/transport, grcp-java devs able to easily use OSGi and remote services. The only thing I'm asking is to (optionally) generate a java interface as part of grcp-java generator plugin rather than having a new/separate tool to do so...thus making it easier by allowing a single service declaration (proto file) to create a complete service api. |
For those interested, I've implemented a service interface generator in this repo: https://github.com/ECF/grpc-osgi-generator It's implemented as a protoc plugin to run alongside protoc with grpc plugin. It only outputs two classes for each service declared in the proto:
This generator is working now in test protos (see generator/examples/hello-world). Of course there is more to do: i.e. test code, docs, examples, travis setup, maven central deployment, possibly more features, etc. I welcome participation and contribution...OSGi focused or not...since as we've discussed here what it generates doesn't depend upon OSGi at all. OSGi Remote Services is my use case at the moment, however. |
The grpc-osgi-generator protoc plugin generates a service interface class with unary and (now) streaming (server streaming, client streaming, and bi-directional streaming). The streaming methods support is provided by the reactive-grpc protoc plugin and so depends upon both reactive-java and reactive-grpc. What this means in practice is that the generated service interface references io.reactivex.Flowable and io.reactivex.Single. The generated interface uses Java8's default keyword so requires Java8 or higher for compile and runtime. Although OSGi is in the name, the grpc-osgi-generator does not require that OSGi be used at all. For OSGi Remote Services, however, there is also now an ECF Remote Services distribution provider that uses this generator in an OSGi Services environment to support interface versioning, pluggable remote service discovery, remote service admin, etc. |
I would like to implement an OSGi Remote Services distribution provider based upon grcp. OSGi Remote Services is a specification:
https://osgi.org/specification/osgi.cmpn/7.0.0/service.remoteservices.html
that allows OSGi services to be exposed for remote access by consumers via a distribution provider. ECF's implementation of remote services:
https://wiki.eclipse.org/ECF
has support for pluggable distribution providers:
https://wiki.eclipse.org/Distribution_Providers
One of these providers uses grcp/protobuf as a distribution provider
https://github.com/ECF/grpc-RemoteServicesProvider
To make this distribution provider easier to use, it would be valuable to be able to have grcp protobuf plugin generate two additional classes:
and the generated Java interface for this proto would be:
Also desirable would be an abstract superclass implementation of this service interface class to allow naive subclass implementations of the remote HealthCheck service, for example:
I've submitted the following pull request as an initial suggested implementation of changes to grcp plugin to support generation of the service interface and abstract impl.
#6980
The text was updated successfully, but these errors were encountered: