Introduce MethodDescriptorSupplier #3086

lukaszx0 · 2017-06-12T05:50:09Z

Second iteration on providing an easier way to access service methods which I took stab at in #2904.

It introduces new MethodDescriptorSupplier and MethodDescriptor#setSchemaDescriptor.

lukaszx0 · 2017-06-12T05:50:38Z

(Splitted into two commit as per @ejona86's suggestion)

ejona86 · 2017-06-12T16:29:37Z

This looks fine to me, other than issues with how the commits were split. Those can either be resolved by reworking what is in each commit or by squashing.

@zhangkun83, @carl-mastrangelo, @dapengzhang0, It's important to note that the new APIs introduced to MethodDescriptor, ProtoServiceDescriptorSupplier, and ProtoMethodDescriptorSupplier can't be @ExperimentalApi since they are used by the codegen. The change to MethodDescriptor is a copy of what is already in ServiceDescriptor.

CC @ericgribkoff

ejona86 · 2017-06-12T16:20:01Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/BenchmarkServiceGrpc.java

@@ -38,6 +38,7 @@ private BenchmarkServiceGrpc() {}
              io.grpc.benchmarks.proto.Messages.SimpleRequest.getDefaultInstance()))
          .setResponseMarshaller(io.grpc.protobuf.ProtoUtils.marshaller(
              io.grpc.benchmarks.proto.Messages.SimpleResponse.getDefaultInstance()))
+          .setSchemaDescriptor(new BenchmarkServiceFileDescriptorSupplier().getServiceDescriptor().findMethodByName("UnaryCall"))


This should have just been .setSchemaDescriptor(new BenchmarkServiceFileDescriptorSupplier())

ejona86 · 2017-06-12T16:22:54Z

protobuf/src/main/java/io/grpc/protobuf/ProtoServiceDescriptorSupplier.java

+/**
+ * Provides access to the underlying proto service descriptor.
+ */
+public interface ProtoServiceDescriptorSupplier extends ProtoFileDescriptorSupplier {


I had intended that this would be in the second commit as well.

lukaszx0 · 2017-06-12T20:00:01Z

@ejona86 squashing will be easier if that works?

lukaszx0 · 2017-06-15T05:31:58Z

@ejona86 rebased and squashed into one commit.

ejona86 · 2017-06-16T21:30:14Z

protobuf/src/main/java/io/grpc/protobuf/ProtoMethodDescriptorSupplier.java

+/**
+ * Provides access to the underlying proto service method descriptor.
+ */
+public interface ProtoMethodDescriptorSupplier {


Extend ProtoServiceDescriptorSupplier?

dapengzhang0 · 2017-06-16T23:23:22Z

LGTM

carl-mastrangelo · 2017-06-19T20:26:00Z

compiler/src/java_plugin/cpp/java_generator.cpp

+        "private static final class $proto_method_descriptor_supplier$ extends $proto_file_descriptor_supplier$ implements $ProtoMethodDescriptorSupplier$ {\n");
+    p->Indent();
+    p->Print(*vars, "private final String methodName;\n\n");
+    p->Print(*vars, "public $proto_method_descriptor_supplier$(String methodName) {\n");


carl-mastrangelo · 2017-06-19T20:30:18Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/BenchmarkServiceGrpc.java

+    }
+  }
+
+  private static final class BenchmarkServiceMethodDescriptorSupplier extends BenchmarkServiceFileDescriptorSupplier implements io.grpc.protobuf.ProtoMethodDescriptorSupplier {


Why extend? What if the service descriptor isn't available? Services have-a set of methods. Methods fail the is-a test. So too with the supplier.

Perhaps a method could be added to get the serviceDescriptorSupplier for the method?

protobuf's MethodDescriptor.getService() is guaranteed to work.

carl-mastrangelo · 2017-06-19T20:33:35Z

core/src/main/java/io/grpc/MethodDescriptor.java

@@ -42,6 +42,7 @@
  private final String fullMethodName;
  private final Marshaller<ReqT> requestMarshaller;
  private final Marshaller<RespT> responseMarshaller;
+  private final Object schemaDescriptor;


@Nullable

carl-mastrangelo · 2017-06-19T20:33:43Z

core/src/main/java/io/grpc/MethodDescriptor.java

+   *
+   * @since 1.5.0
+   */
+  public Object getSchemaDescriptor() {


@Nullable

carl-mastrangelo · 2017-06-19T20:35:17Z

protobuf/src/main/java/io/grpc/protobuf/ProtoMethodDescriptorSupplier.java

+/**
+ * Provides access to the underlying proto service method descriptor.
+ */
+public interface ProtoMethodDescriptorSupplier {


@DoNotMock

DoNotMock an interface? What would break?

carl-mastrangelo · 2017-06-19T20:35:27Z

protobuf/src/main/java/io/grpc/protobuf/ProtoMethodDescriptorSupplier.java

+  /**
+   * Returns method descriptor to the proto service method.
+   */
+  MethodDescriptor getMethodDescriptor();


@CheckReturnValue

carl-mastrangelo · 2017-06-19T20:38:20Z

compiler/src/java_plugin/cpp/java_generator.cpp

+
+    p->Print(
+        *vars,
+        "private static final class $proto_method_descriptor_supplier$ extends $proto_file_descriptor_supplier$ implements $ProtoMethodDescriptorSupplier$ {\n");


So far grpc generated code is fairly readable. One of the things that makes it that way is generally fitting within line limits. Until we auto format code, could this be wrapped to try to fit? Google style is binary ops go on the following line. Something like:

class Foo extends Bar implements Baz

carl-mastrangelo · 2017-06-19T20:40:26Z

I'm curious how this will interact with the existing schema objects for marshallers. What should the relationship be? (can one be null but not the other? Vice-versa?, Should marshaller schema be removed?)

ejona86 · 2017-06-20T15:58:16Z

@carl-mastrangelo, there aren't schema objects for marshallers. There are schema objects for services though. It seems necessary to have a separate places to store them. We could use the same schema object for both, though (although then it takes more work to lookup the proto MethodDescriptor).

lukaszx0 · 2017-06-21T01:50:55Z

@carl-mastrangelo addressed your comments, PTAL

ejona86 · 2017-07-20T19:34:33Z

@carl-mastrangelo, ping

carl-mastrangelo · 2017-07-22T03:16:55Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/WorkerServiceGrpc.java

@@ -392,11 +396,32 @@ public void invoke(Req request, io.grpc.stub.StreamObserver<Resp> responseObserv
    }
  }

-  private static final class WorkerServiceDescriptorSupplier implements io.grpc.protobuf.ProtoFileDescriptorSupplier {
+  private static class WorkerServiceFileDescriptorSupplier


I would prefer if this class could be final, since every other class in this generate code is final too.

What do you think about making this final, making a static instance on the MethodDescriptorSupplier, and having MethodDescriptorSupplier implement the interfaces here?

(non final classes, even private ones can be mocked by spying instances of them. This was the cause of a recent test breakage)

carl-mastrangelo · 2017-07-22T03:17:48Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/WorkerServiceGrpc.java

    @java.lang.Override
    public com.google.protobuf.Descriptors.FileDescriptor getFileDescriptor() {
      return io.grpc.benchmarks.proto.Services.getDescriptor();
    }
+


This need a package-private constructor. A private ctor causes synthetic classes to be made, and a public one makes reflection too easy.

carl-mastrangelo · 2017-07-22T03:18:38Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/WorkerServiceGrpc.java

+        implements io.grpc.protobuf.ProtoMethodDescriptorSupplier {
+    private final String methodName;
+
+    private WorkerServiceMethodDescriptorSupplier(String methodName) {


This should be package-protected too.

carl-mastrangelo · 2017-07-22T03:19:36Z

benchmarks/src/generated/main/grpc/io/grpc/benchmarks/proto/WorkerServiceGrpc.java

+      extends WorkerServiceFileDescriptorSupplier
+        implements io.grpc.protobuf.ProtoMethodDescriptorSupplier {
+    private final String methodName;
+


Another alternative would be to nest the MethodDescriptorSupplier class as an inner class of the FileDescriptorSupplier class, and let it capture the fields and methods.

ejona86 · 2017-08-10T21:48:01Z

@lukaszx0, it seems there are only small changes remaining. Want to fix them up, or us to?

ejona86 · 2017-08-25T22:35:27Z

@carl-mastrangelo, PTAL. Does this work for you? I created a base abstract class and extend it for both file and method descriptors, which are then both final.

I also took the liberty to make java_generator.cpp use a giant string for the block of code, which makes it much more readable and easier to modify for me.

ejona86 · 2017-09-01T19:33:24Z

@carl-mastrangelo, friendly ping.

carl-mastrangelo · 2017-09-01T21:41:03Z

core/src/main/java/io/grpc/MethodDescriptor.java

+   * sets this value, in order to be consumed by the server reflection service.  See also:
+   * {@code io.grpc.protobuf.ProtoMethodDescriptorSupplier}.
+   *
+   * @since 1.5.0


carl-mastrangelo

A few minor comments but LGTM

carl-mastrangelo · 2017-09-01T21:41:29Z

protobuf/src/main/java/io/grpc/protobuf/ProtoMethodDescriptorSupplier.java

+
+/**
+ * Provides access to the underlying proto service method descriptor.
+ */


Would you mind adding a @Since here too?

carl-mastrangelo · 2017-09-01T21:41:36Z

protobuf/src/main/java/io/grpc/protobuf/ProtoServiceDescriptorSupplier.java

+
+/**
+ * Provides access to the underlying proto service descriptor.
+ */


ejona86 · 2017-09-08T23:51:24Z

@carl-mastrangelo, can you review #3451? It includes the fixes requested (plus another one I noticed). I couldn't push to lukaszx0's branch, so I needed to create a new PR.

ejona86 · 2017-09-11T16:57:21Z

Merged as part of #3451

lukaszx0 mentioned this pull request Jun 12, 2017

Service Method Options (Protobuf) #2904

Closed

ejona86 reviewed Jun 12, 2017

View reviewed changes

core, compiler, protobuf: introduce MethodDescriptor#setSchemaDescriptor

1f588fd

lukaszx0 force-pushed the lukasz/method-descriptor branch from 136558d to 1f588fd Compare June 15, 2017 05:31

ejona86 approved these changes Jun 16, 2017

View reviewed changes

carl-mastrangelo reviewed Jun 19, 2017

View reviewed changes

compiler: add @nullable, improve formatting

18dd85a

carl-mastrangelo reviewed Jul 22, 2017

View reviewed changes

Avoid returning non-final classes; avoid synthetic classes

3e69624

carl-mastrangelo reviewed Sep 1, 2017

View reviewed changes

carl-mastrangelo approved these changes Sep 1, 2017

View reviewed changes

ejona86 mentioned this pull request Sep 8, 2017

Introduce MethodDescriptorSupplier #3451

Merged

ejona86 closed this Sep 11, 2017

lock bot locked and limited conversation to collaborators Sep 21, 2018

Introduce MethodDescriptorSupplier #3086

Introduce MethodDescriptorSupplier #3086

Conversation

lukaszx0 commented Jun 12, 2017

lukaszx0 commented Jun 12, 2017

ejona86 commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszx0 commented Jun 12, 2017

lukaszx0 commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapengzhang0 commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carl-mastrangelo commented Jun 19, 2017

ejona86 commented Jun 20, 2017

lukaszx0 commented Jun 21, 2017

ejona86 commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Aug 10, 2017

ejona86 commented Aug 25, 2017

ejona86 commented Sep 1, 2017

Choose a reason for hiding this comment

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Sep 8, 2017 • edited

ejona86 commented Sep 11, 2017

ejona86 commented Sep 8, 2017 •

edited