Moving code from gax-grpc to gax by garrettjonesgoogle · Pull Request #359 · googleapis/gax-java

garrettjonesgoogle · 2017-08-25T00:41:18Z

~~This is just a work-in-progress PR, so I am just looking for feedback on overall design.~~ I'd like validation on the following changes:

Overall, I tried to move logic up from grpc/httpjson to gax.rpc
Moved most of the implementation of GrpcCallableFactory to CallableFactory in gax.rpc
Created an OperationApi abstraction in gax.rpc, along with an OperationSnapshot abstraction
3.1. Now that OperationSnapshot is used throughout gax.rpc, OperationT is removed as a type parameterization
3.2. Most of the operation implementation is moved up to gax.rpc
Renamed Transport to TransportChannel and ChannelProvider to TransportChannelProvider; removed TransportProvider, and instead repurposed Instantiating(*)ChannelProvider to directly create TransportChannel objects (either GrpcTransportChannel or HttpJsonTransportChannel), cutting out a layer of abstraction
Created new TransportDescriptor concept, to allow for transport-specific logic to be supplied in various places
Moved header logic out of Instantiating(*)ChannelProvider into a new HeaderProvider, which lives at the gax.rpc level and can be shared by grpc & http/json
In the new TransportChannelProvider, changed the model of providing other providers, such that you can do provider.withX(X x), so now there is only a single getTransportChannel() method

codecov-io · 2017-09-05T19:13:20Z

Codecov Report

Merging #359 into gax-httpjson will increase coverage by 5.55%.
The diff coverage is 74.45%.

@@                Coverage Diff                 @@
##             gax-httpjson     #359      +/-   ##
==================================================
+ Coverage           64.81%   70.37%   +5.55%     
- Complexity            541      624      +83     
==================================================
  Files                 151      150       -1     
  Lines                3203     2994     -209     
  Branches              237      230       -7     
==================================================
+ Hits                 2076     2107      +31     
+ Misses               1026      795     -231     
+ Partials              101       92       -9

Impacted Files	Coverage Δ	Complexity Δ
...oogle/api/gax/httpjson/ManagedHttpJsonChannel.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...om/google/api/gax/rpc/ApiResultRetryAlgorithm.java	`90.9% <ø> (ø)`	`7 <0> (?)`
...m/google/api/gax/httpjson/ApiMethodDescriptor.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`40.9% <ø> (ø)`	`5 <0> (ø)`	⬇️
...ogle/api/gax/httpjson/HttpJsonCallableFactory.java	`0% <ø> (-38.6%)`	`0 <0> (-6)`
...m/google/api/gax/httpjson/HttpJsonCallOptions.java	`81.81% <ø> (ø)`	`4 <0> (ø)`	⬇️
...ogle/api/gax/httpjson/HttpJsonHeaderEnhancers.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...java/com/google/api/gax/rpc/OperationCallable.java	`100% <ø> (ø)`	`6 <0> (ø)`	⬇️
...oogle/api/gax/rpc/EntryPointOperationCallable.java	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`80% <ø> (-5.9%)`	`4 <0> (-11)`
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d720bb...d913a3f. Read the comment docs.

pongad · 2017-09-08T04:00:14Z

Can this be split at all? The individual bullets in the description sound reasonable, but I'm having great difficulty groking the 7000 line diff

vam-google

(sorry, I had to modify your initial PR comment from bulleted list to numbered list, to reference the items here)

Overall looks good to me.
Cool!
Please check the GrpcOperationApi class review comments for more details. Also OperationApi name is weird. What does it even mean? Is it an operation client? A wrapper around it? A bunch of transformers? "Api" suffix is too generic.
The http/grpc difference is now mostly on channel level, I believe it is good.
The descriptor looks like a class which encapsulates random transport-specific stuff. It doesn't look very pretty, but seems unavoidable, so I'm fine with it.
Please check the header provider specific comments for more details. In general looks good, but I'm still not sure how well that would fit in header routing problem. In general we need a good way to create 2 types of headers: static (same for same client, not recalculated every time) and dynamic (different per request, recalculated for each request). For dynamic headers on the moment of creation there should be access to the corresponding parameter values (so we can put it in headers). Propagating that to such a low level can be problematic.
Not really sure what your comment means here :).

gax/src/main/java/com/google/api/gax/rpc/OperationApi.java

-/** The settings used to create a Transport. */
 @BetaApi
-public interface TransportProvider {
+public interface OperationApi {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcOperationSnapshot.java

-@BetaApi
-public final class FixedChannelProvider implements ChannelProvider {
-  private final ManagedChannel channel;
+public class GrpcOperationSnapshot implements OperationSnapshot {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcOperationTransformers.java

+import io.grpc.Status.Code;
+
+/** Package-private for internal use. */
+class GrpcOperationTransformers {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcStatusCodes.java

+
+  public static Set<Code> getGrpcStatusCodes(Set<StatusCode> statusCodes) {
+    Set<Status.Code> returnCodes = new HashSet<>();
+    for (StatusCode code : statusCodes) {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportDescriptor.java

+    exceptionContext.getResultFuture().setException(exceptionToThrow);
+  }
+
+  public ApiCallContext getDefaultCallContext() {


gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java

+    initialContext = getClientContext(initialChannel, executor);
+  }
+
+  private static class ResponseTransformer implements ApiFunction<OperationSnapshot, Color> {


gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java

+      public ApiFuture<OperationSnapshot> futureCall(RequestT request, ApiCallContext context) {
+        OperationSnapshot response = results[index];
+        if (index < results.length - 1) {
+          index += 1;


gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java

+      case CANCELLED:
+        expectedClass = CancelledException.class;
+        break;
+        //      case NOT_FOUND:


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcOperationApi.java

+            return CancelOperationRequest.newBuilder().setName(request).build();
+          }
+        },
+        new ApiFunction<Empty, Void>() {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcOperationApi.java

+        new ApiFunction<Operation, OperationSnapshot>() {
+          @Override
+          public OperationSnapshot apply(Operation operation) {
+            return GrpcOperationSnapshot.create(operation);


garrettjonesgoogle · 2017-09-25T23:25:20Z

Header routing problem: my implementation is for static headers. I think we can make an additive change later for dynamic headers (which you would need).
Look at the withExecutor() and withHeaders() methods on TransportChannelProvider, and see if it makes sense to you.

garrettjonesgoogle · 2017-09-25T23:28:40Z

I have addressed feedback from @vam-google . PTAL if you want, or wait for a following update where I update docs & add tests.

googlebot · 2017-09-27T22:55:26Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-09-28T18:22:51Z

CLAs look good, thanks!

garrettjonesgoogle · 2017-09-30T00:06:41Z

@vam-google I have updated docs & tests, PTAL

garrettjonesgoogle · 2017-10-02T20:24:17Z

I have created a diff PR for certain files as promised: #374

Note, it only include files that git didn't match up correctly; it doesn't include all files that were touched in the present PR.

garrettjonesgoogle · 2017-10-02T20:31:32Z

@pongad I would recommend doing multiple passes on this PR and focus on different things in those passes; it would be hard to split this PR apart because it involves code moves & renames, so any splits would result in code that wouldn't compile.

One way to cut down on the lines to review is that you could completely skip gax-httpjson changes; they will mirror gax-grpc directly. Secondly, the most important thing to focus on is the surface under gax/ (transport-agnostic surfaces); prioritize reviewing that code.

Lastly, the diff PR should help reviewing some files that git didn't match up for diffs.

pongad

A couple nits, but otherwise I have no specific concern with this.

There's a lot of code here though, which makes me wonder if the callable stack was the right design for this problem. (Not blaming anyone; in fact I'm probably the one most responsible for it.) If we ever need to add a third transport, we should probably revisit the design.

gax/src/main/java/com/google/api/gax/rpc/TransportDescriptor.java

+        "TransportDescriptor.createDefaultCallContext not implemented");
+  }
+
+  public ApiCallContext getCallContextWithDefault(ApiCallContext inputContext) {


gax/src/main/java/com/google/api/gax/rpc/NoHeaderProvider.java

+
+  @Override
+  public Map<String, String> getHeaders() {
+    return new HashMap<>();


garrettjonesgoogle · 2017-10-03T23:07:48Z

@pongad I believe the callable stack was the right design. We can't really revisit the design if we want a third transport because we're going GA before such a thing will even be considered. Register your concerns now w.r.t. supporting a third transport!

pongad · 2017-10-04T01:42:27Z

@garrettjonesgoogle I don't have specific concerns about adding new transports. Not having worked on gax-java for a while, I find it difficult to think of specific action items. I'm sorry I'm not being helpful, but here's my "nebulous" concern.

At the time of writing, gax-java master has 212 files and ~13,000 lines of code. At the same time, gax-go has 250 statements. I understand LOC isn't a good measure for quality or productivity, and to be fair, Java's job is harder (eg, Go can efficiently block; Java has to do things with executors). I don't think this should block GA, but there's something unsettling about the 52x size difference. Maybe this is just me being scared by seeing all this code at once...

garrettjonesgoogle · 2017-10-09T16:54:27Z

@vam-google , I have done the following:

Added initialFuture back to OperationFuture
Moved the Fake* classes to src/test

As I mentioned in the diff PR, there is a test for the transformers at GrpcOperationTransformersTest .

Is there anything else I'm missing?

I'd like to merge this to the gax-httpjson branch and then do another PR with additional changes before I merge to master.

vam-google

LGTM
(please push in a non-master branch)

garrettjonesgoogle force-pushed the gax-httpjson branch 2 times, most recently from 6bec56a to ebb2cf9 Compare September 5, 2017 18:31

WIP moving code from gax-grpc to gax

ba3f3ad

garrettjonesgoogle force-pushed the gax-httpjson branch from 9db1edc to ba3f3ad Compare September 5, 2017 19:11

garrettjonesgoogle requested review from pongad and vam-google September 5, 2017 20:35

vam-google reviewed Sep 19, 2017

View reviewed changes

Addressing PR feedback

8afc3c4

garrettjonesgoogle added 2 commits September 26, 2017 13:19

Adding docs

77dcec7

Moving tests, adding tests

6b1d29d

More tests, test fixes

a401e67

garrettjonesgoogle added 4 commits September 28, 2017 15:15

More tests for gax-grpc

48a7081

progress commit

8d45d2d

More tests, deleting unused code

d650c78

Fixing docs and last FIXMEs

9d451f3

garrettjonesgoogle changed the title ~~WIP moving code from gax-grpc to gax~~ Moving code from gax-grpc to gax Sep 30, 2017

Removing star import (Darn IntelliJ!)

13df9fb

garrettjonesgoogle mentioned this pull request Oct 2, 2017

[DIFF PR] showing diffs for moving code from gax-grpc to gax #374

Closed

pongad reviewed Oct 3, 2017

View reviewed changes

garrettjonesgoogle added 2 commits October 6, 2017 14:09

Adding GrpcOperationSnapshotCallable

d665078

Addressing PR feedback

d913a3f

vam-google approved these changes Oct 9, 2017

View reviewed changes

garrettjonesgoogle merged commit bfdfd55 into googleapis:gax-httpjson Oct 9, 2017

garrettjonesgoogle added a commit to garrettjonesgoogle/gax-java that referenced this pull request Oct 18, 2017

Moving code from gax-grpc to gax (googleapis#359)

bbaa1e9

garrettjonesgoogle added a commit that referenced this pull request Oct 31, 2017

Moving code from gax-grpc to gax (#359)

ecb4e74

garrettjonesgoogle added a commit that referenced this pull request Oct 31, 2017

Moving code from gax-grpc to gax (#359)

5087e39

Conversation

garrettjonesgoogle commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pongad commented Sep 8, 2017

Uh oh!

vam-google left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Sep 25, 2017

Uh oh!

garrettjonesgoogle commented Sep 25, 2017

Uh oh!

googlebot commented Sep 27, 2017

Uh oh!

googlebot commented Sep 28, 2017

Uh oh!

garrettjonesgoogle commented Sep 30, 2017

Uh oh!

garrettjonesgoogle commented Oct 2, 2017

Uh oh!

garrettjonesgoogle commented Oct 2, 2017

Uh oh!

pongad left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

garrettjonesgoogle commented Aug 25, 2017 •

edited

Loading

codecov-io commented Sep 5, 2017 •

edited

Loading

vam-google left a comment •

edited

Loading

pongad commented Oct 4, 2017 •

edited

Loading