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

Add Abstract{ServiceName} class in compiled service class #1634

Closed
wants to merge 1 commit into
base: master
from
Jump to file or symbol
Failed to load files and symbols.
+148 −0
Diff settings

Always

Just for now

@@ -157,6 +157,12 @@ protected BenchmarkServiceFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractBenchmarkService implements BenchmarkService, io.grpc.stub.BindableService {

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

I am having a hard time understanding what the value add is here. Would you be able to add examples classes that show off how this would be used? Right now this just seems to be a minor convenience class.

I am pretty reluctant to increase our API surface without some real, tangible benefit.

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

I am having a hard time understanding what the value add is here. Would you be able to add examples classes that show off how this would be used? Right now this just seems to be a minor convenience class.

I am pretty reluctant to increase our API surface without some real, tangible benefit.

This comment has been minimized.

@lukaszx0

lukaszx0 Apr 7, 2016

Collaborator

@carl-mastrangelo One use case, on which I'm blocked because of lack functionality like this, is in our framework where we define exposed grpc services via annotations. Simplified example:

@App("route-guide")
@ExposedGrpcService(RouteGuideService.class)
class RouteGuideApp {
  ...
}

class RouteGuideService extends AbstractRouteGuide {
 ...
}

With BindableService interface, we can do strong typing for value in @ExposedGrpcService - Class<? extends BindableService> - and avoid using reflection or other weird and tangled logic to build grpc server, it'll be as simple as:

ServerBuilder.forPort(...).addService(exposedGrpcServiceAnnotation.value().bindService())

I can imagine people having a number of similar use cases in their frameworks/apps where richer type for service implementation will be beneficial. It will also be helpful when using DI frameworks etc.

@lukaszx0

lukaszx0 Apr 7, 2016

Collaborator

@carl-mastrangelo One use case, on which I'm blocked because of lack functionality like this, is in our framework where we define exposed grpc services via annotations. Simplified example:

@App("route-guide")
@ExposedGrpcService(RouteGuideService.class)
class RouteGuideApp {
  ...
}

class RouteGuideService extends AbstractRouteGuide {
 ...
}

With BindableService interface, we can do strong typing for value in @ExposedGrpcService - Class<? extends BindableService> - and avoid using reflection or other weird and tangled logic to build grpc server, it'll be as simple as:

ServerBuilder.forPort(...).addService(exposedGrpcServiceAnnotation.value().bindService())

I can imagine people having a number of similar use cases in their frameworks/apps where richer type for service implementation will be beneficial. It will also be helpful when using DI frameworks etc.

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

Internally at Google (and strongly related to #1469) we have Abstract class versions called BaseRouteGuideService or the like, which basically protects clients from interface changes breaking their build. The default just fails the rpc immediately while allowing the code to still compile.

Now, with that in mind, how does your change fit into this? You have a new abstract class that does something that looks like it saves you a few lines, and is another abstract class that is in our interface. Which abstract class is the correct one to use? The answer isn't "just tab complete it" any more, which means now people will have to look into the generated code to see which one is right. That cost is too much. To add to the API, there needs to be really outstanding gains, on the order of making something impossible possible. How does this the vast majority of people who aren't using an annotation based service binding?

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

Internally at Google (and strongly related to #1469) we have Abstract class versions called BaseRouteGuideService or the like, which basically protects clients from interface changes breaking their build. The default just fails the rpc immediately while allowing the code to still compile.

Now, with that in mind, how does your change fit into this? You have a new abstract class that does something that looks like it saves you a few lines, and is another abstract class that is in our interface. Which abstract class is the correct one to use? The answer isn't "just tab complete it" any more, which means now people will have to look into the generated code to see which one is right. That cost is too much. To add to the API, there needs to be really outstanding gains, on the order of making something impossible possible. How does this the vast majority of people who aren't using an annotation based service binding?

This comment has been minimized.

@ejona86

ejona86 Apr 7, 2016

Member

@carl-mastrangelo, I was thinking that the abstract class with default method implementations could be the same base class as here. It'd be what we suggest everyone to use. I'd also expect a ServerBuilder.addService(BindableService) convenience overload.

Any trailing concerns I have are more nit-realm, like does "bind" still make sense (vs, say, getDescriptor) or should bindService() be able to return multiple descriptors. I didn't bother mentioning them, but if a tweak like that would make the change make more sense we could talk about it.

@ejona86

ejona86 Apr 7, 2016

Member

@carl-mastrangelo, I was thinking that the abstract class with default method implementations could be the same base class as here. It'd be what we suggest everyone to use. I'd also expect a ServerBuilder.addService(BindableService) convenience overload.

Any trailing concerns I have are more nit-realm, like does "bind" still make sense (vs, say, getDescriptor) or should bindService() be able to return multiple descriptors. I didn't bother mentioning them, but if a tweak like that would make the change make more sense we could talk about it.

This comment has been minimized.

@lukaszx0

lukaszx0 Apr 7, 2016

Collaborator

I see what you're saying. Are BaseRouteGuideService classes that you're using at Google providing something else besides just implementing service interface and returning UNIMPLEMENTED by default? Are they adding some base functionality shared across service methods? Would implementing those methods in new AbstractRouteGuideService eliminate the need of having BaseRouteGuideService?

I must say I quickly skimmed through #1469 at first and didn't fully understand it. I see the point of having "implemented-unimplemented" base classes now and I think we should implement this. I'll add this to my PR.

@lukaszx0

lukaszx0 Apr 7, 2016

Collaborator

I see what you're saying. Are BaseRouteGuideService classes that you're using at Google providing something else besides just implementing service interface and returning UNIMPLEMENTED by default? Are they adding some base functionality shared across service methods? Would implementing those methods in new AbstractRouteGuideService eliminate the need of having BaseRouteGuideService?

I must say I quickly skimmed through #1469 at first and didn't fully understand it. I see the point of having "implemented-unimplemented" base classes now and I think we should implement this. I'll add this to my PR.

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

Ok, I can get behind this change.

For selfish reasons I would prefer to name this class BaseFooService, since that is what we name the analog internally. I am probably going to be addding changes that add "unimplemented" method overrides to this class in a soon to be created PR.

@carl-mastrangelo

carl-mastrangelo Apr 7, 2016

Member

Ok, I can get behind this change.

For selfish reasons I would prefer to name this class BaseFooService, since that is what we name the analog internally. I am probably going to be addding changes that add "unimplemented" method overrides to this class in a soon to be created PR.

@Override public io.grpc.ServerServiceDefinition bindService() {
return BenchmarkServiceGrpc.bindService(this);
}
}
private static final int METHODID_UNARY_CALL = 0;
private static final int METHODID_STREAMING_CALL = 1;
@@ -213,6 +213,12 @@ protected WorkerServiceFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractWorkerService implements WorkerService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return WorkerServiceGrpc.bindService(this);
}
}
private static final int METHODID_CORE_COUNT = 0;
private static final int METHODID_QUIT_WORKER = 1;
private static final int METHODID_RUN_SERVER = 2;
@@ -642,6 +642,27 @@ static void PrintBindServiceMethod(const ServiceDescriptor* service,
p->Print("}\n");
}
static void PrintAbstractServiceClass(const ServiceDescriptor* service,
map<string, string>* vars,
Printer* p) {
p->Print(
*vars,
"public static abstract class Abstract$service_name$"
" implements $service_name$, $BindableService$ {\n");
p->Indent();
p->Print(*vars,
"@Override public $ServerServiceDefinition$ bindService() {\n"
);
p->Indent();
p->Print(*vars,
"return $service_class_name$.bindService(this);\n"
);
p->Outdent();
p->Print("}\n");
p->Outdent();
p->Print("}\n\n");
}
static void PrintService(const ServiceDescriptor* service,
map<string, string>* vars,
Printer* p,
@@ -704,6 +725,7 @@ static void PrintService(const ServiceDescriptor* service,
PrintStub(service, vars, p, ASYNC_CLIENT_IMPL, generate_nano);
PrintStub(service, vars, p, BLOCKING_CLIENT_IMPL, generate_nano);
PrintStub(service, vars, p, FUTURE_CLIENT_IMPL, generate_nano);
PrintAbstractServiceClass(service, vars, p);
PrintMethodHandlerClass(service, vars, p, generate_nano);
PrintBindServiceMethod(service, vars, p, generate_nano);
p->Outdent();
@@ -754,6 +776,7 @@ void GenerateService(const ServiceDescriptor* service,
vars["MethodType"] = "io.grpc.MethodDescriptor.MethodType";
vars["ServerMethodDefinition"] =
"io.grpc.ServerMethodDefinition";
vars["BindableService"] = "io.grpc.stub.BindableService";
vars["ServerServiceDefinition"] =
"io.grpc.ServerServiceDefinition";
vars["AbstractStub"] = "io.grpc.stub.AbstractStub";
@@ -224,6 +224,12 @@ public class TestServiceGrpc {
}
}
public static abstract class AbstractTestService implements TestService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return TestServiceGrpc.bindService(this);
}
}
private static final int METHODID_UNARY_CALL = 0;
private static final int METHODID_STREAMING_OUTPUT_CALL = 1;
private static final int METHODID_STREAMING_INPUT_CALL = 2;
@@ -224,6 +224,12 @@ public class TestServiceGrpc {
}
}
public static abstract class AbstractTestService implements TestService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return TestServiceGrpc.bindService(this);
}
}
private static final int METHODID_UNARY_CALL = 0;
private static final int METHODID_STREAMING_OUTPUT_CALL = 1;
private static final int METHODID_STREAMING_INPUT_CALL = 2;
@@ -302,6 +302,12 @@ public class TestServiceGrpc {
}
}
public static abstract class AbstractTestService implements TestService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return TestServiceGrpc.bindService(this);
}
}
private static final int METHODID_UNARY_CALL = 0;
private static final int METHODID_STREAMING_OUTPUT_CALL = 1;
private static final int METHODID_STREAMING_INPUT_CALL = 2;
@@ -138,6 +138,12 @@ protected GreeterFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractGreeter implements Greeter, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return GreeterGrpc.bindService(this);
}
}
private static final int METHODID_SAY_HELLO = 0;
private static class MethodHandlers<Req, Resp> implements
@@ -205,6 +205,12 @@ protected RouteGuideFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractRouteGuide implements RouteGuide, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return RouteGuideGrpc.bindService(this);
}
}
private static final int METHODID_GET_FEATURE = 0;
private static final int METHODID_LIST_FEATURES = 1;
private static final int METHODID_RECORD_ROUTE = 2;
@@ -120,6 +120,12 @@ protected LoadBalancerFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractLoadBalancer implements LoadBalancer, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return LoadBalancerGrpc.bindService(this);
}
}
private static final int METHODID_BALANCE_LOAD = 0;
private static class MethodHandlers<Req, Resp> implements
@@ -175,6 +175,12 @@ protected ReconnectServiceFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractReconnectService implements ReconnectService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return ReconnectServiceGrpc.bindService(this);
}
}
private static final int METHODID_START = 0;
private static final int METHODID_STOP = 1;
@@ -261,6 +261,12 @@ protected TestServiceFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractTestService implements TestService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return TestServiceGrpc.bindService(this);
}
}
private static final int METHODID_EMPTY_CALL = 0;
private static final int METHODID_UNARY_CALL = 1;
private static final int METHODID_STREAMING_OUTPUT_CALL = 2;
@@ -138,6 +138,12 @@ protected UnimplementedServiceFutureStub build(io.grpc.Channel channel,
}
}
public static abstract class AbstractUnimplementedService implements UnimplementedService, io.grpc.stub.BindableService {
@Override public io.grpc.ServerServiceDefinition bindService() {
return UnimplementedServiceGrpc.bindService(this);
}
}
private static final int METHODID_UNIMPLEMENTED_CALL = 0;
private static class MethodHandlers<Req, Resp> implements
@@ -0,0 +1,59 @@
/*
* Copyright 2016, Google Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
*
* * Neither the name of Google Inc. nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package io.grpc.stub;
import io.grpc.ExperimentalApi;
import io.grpc.ServerServiceDefinition;
/**
* Provides a way to bind instance of service implementation to server.
*
* <p>Enables dynamic creation of {@link ServerServiceDefinition} when exact type of service class
* is unknown.
*
* <pre><code>Class<? extends BindableService> service = ...;
* ServerBuilder.forPort(...).addService(service.newInstance().bindService());
* </code></pre></p>
*
* <p>It is used by service's abstract class generated by compiler (eg.:
* RouteGuideGrpc.AbstractRouteGuide for RouteGuide service). Service implementation classes which
* want to be "dynamically" bind should inherit from this base class.</p>
*/
@ExperimentalApi
public interface BindableService {
/**
* Creates {@link ServerServiceDefinition} object for current instance of service implementation.
*
* @return ServerServiceDefinition object.
*/
ServerServiceDefinition bindService();
}
ProTip! Use n and p to navigate between commits in a pull request.