feat(operations): Add WaitOperation API surface [gax-java] #1284

miraleung · 2021-01-28T22:49:13Z

No description provided.

… fix/lro-headers

codecov · 2021-01-28T22:50:59Z

Codecov Report

Merging #1284 (a82b67e) into master (348f136) will increase coverage by 0.04%.
The diff coverage is 88.57%.

@@             Coverage Diff              @@
##             master    googleapis/gax-java#1284      +/-   ##
============================================
+ Coverage     79.47%   79.52%   +0.04%     
- Complexity     1235     1239       +4     
============================================
  Files           209      209              
  Lines          5399     5426      +27     
  Branches        454      451       -3     
============================================
+ Hits           4291     4315      +24     
- Misses          929      933       +4     
+ Partials        179      178       -1

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/google/longrunning/OperationsSettings.java	`27.27% <0.00%> (-1.76%)`	`2.00 <0.00> (ø)`
...va/com/google/longrunning/stub/OperationsStub.java	`14.28% <0.00%> (-2.39%)`	`1.00 <0.00> (ø)`
...oogle/longrunning/stub/OperationsStubSettings.java	`75.00% <88.88%> (+1.16%)`	`9.00 <1.00> (+1.00)`
.../java/com/google/longrunning/OperationsClient.java	`74.19% <100.00%> (+0.86%)`	`18.00 <2.00> (+2.00)`
...om/google/longrunning/stub/GrpcOperationsStub.java	`92.96% <100.00%> (+1.37%)`	`12.00 <1.00> (+1.00)`
.../com/google/api/gax/httpjson/HttpHeadersUtils.java	`84.61% <0.00%> (-1.10%)`	`11.00% <0.00%> (ø%)`
...m/google/api/gax/retrying/BasicRetryingFuture.java	`93.06% <0.00%> (-0.07%)`	`25.00% <0.00%> (ø%)`
...le/api/gax/retrying/ExponentialRetryAlgorithm.java	`96.29% <0.00%> (-0.07%)`	`14.00% <0.00%> (ø%)`
.../java/com/google/api/gax/batching/BatcherImpl.java	`96.71% <0.00%> (-0.07%)`	`16.00% <0.00%> (ø%)`
...le/api/gax/httpjson/HttpJsonExceptionCallable.java	`81.57% <0.00%> (+2.09%)`	`2.00% <0.00%> (ø%)`

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 348f136...a82b67e. Read the comment docs.

elharo · 2021-01-28T23:06:59Z

gax-grpc/src/main/java/com/google/longrunning/OperationsClient.java

@@ -519,6 +519,61 @@ private final void deleteOperation(DeleteOperationRequest request) {
    return stub.deleteOperationCallable();
  }

+  /**
+   * Waits for the specified long-running operation until it is done or reaches at most a specified


"Waits for the specified long-running operation until it is" --> "Waits until the specified long-running operation is"

This is meant to reflect the RPC's comment verbatim. If I change it here, we'll need to update the protos as well.

Is this PR checking in generated code? I'm not sure we need to reflect the RPC's comment verbatim (though, yes, it should be changed there too.)

The delta here is nearly identical to the generated code, so yes. I'm adding it in this way (as opposed to copy-pasting the new GAPIC client entirely) to preserve existing manual changes and ensure that dependencies won't break.

Updated the comment.

elharo · 2021-01-28T23:07:16Z

gax-grpc/src/main/java/com/google/longrunning/OperationsClient.java

+   * }
+   * </code></pre>
+   *
+   * @param request The request object containing all of the parameters for the API call.


The --> the
no period

need to push?

Pushed, thanks.

The --> the
no period
per Oracle JavaDoc guidelines

Done. The rest of the comments in this file won't match, but that's fine.

elharo · 2021-01-28T23:07:21Z

gax-grpc/src/main/java/com/google/longrunning/OperationsClient.java

+  }
+
+  /**
+   * Waits for the specified long-running operation until it is done or reaches at most a specified


"Waits for the specified long-running operation until it is" --> "Waits until the specified long-running operation is"

Same as above.

elharo · 2021-01-28T23:07:49Z

gax-grpc/src/main/java/com/google/longrunning/OperationsSettings.java

@@ -72,6 +72,11 @@
    return ((OperationsStubSettings) getStubSettings()).deleteOperationSettings();
  }

+  /** Returns the object with the settings used for calls to waitOperation. */
+  public UnaryCallSettings<WaitOperationRequest, Operation> waitOperationSettings() {


this looks testable

This is a direct import of the generated GAPIC surface for the Wait* portion of the LRO proto, so none of the *Settings() getters in this file are tested (as is the case in all the generated GAPIC libraries).

I'm trying to understand whether you're proposing re-generating code in the future. That's not longterm maintainable. There are two paths that can work. Either the code is generated every time when the project is compiled, with the generated code never checked into the repo (the protobuf approach) or it's generated once, checked into the repo, and then manually edited for ever after. Anything in between makes maintenance extremely difficult. Which path is this PR proposing?

Ping. This is a critical issue.

This PR (and others in this repo) only updates the LRO client with the auto-generated section for Wait, and does not propose a particular path. While the long-term path is not clear yet, keeping this client as consistent as possible with the generated code will make debugging less error prone and make updates easier, while we figure this out.

Autogenerated or no., I still think a test helps here. If the autogenerated code chages in a way that causes the test to fail, the test will make that apparent.

I've filed #1299 to track this, since adding tests for this method is essentially equivalent to setting up tests for all the settings getters, which is beyond the scope of this PR.

This is a great point, and I'll look into whether we can add such autogenerated tests to gapic-generator-java.

elharo · 2021-01-28T23:07:59Z

gax-grpc/src/main/java/com/google/longrunning/OperationsSettings.java

@@ -170,6 +175,11 @@ public Builder applyToAllUnaryMethods(
      return getStubSettingsBuilder().deleteOperationSettings();
    }

+    /** Returns the builder for the settings used for calls to deleteOperation. */
+    public UnaryCallSettings.Builder<WaitOperationRequest, Operation> waitOperationSettings() {


I still think there should be a test of this method.

As above - tracked in #1299, will investigate this feature for gapic-generator-java.

elharo · 2021-01-28T23:08:31Z

gax-grpc/src/main/java/com/google/longrunning/stub/OperationsStub.java

@@ -74,6 +75,10 @@
    throw new UnsupportedOperationException("Not implemented: deleteOperationCallable()");
  }

+  public UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable() {


@Override

I don't think there's a corresponding method in BackgroundResource? Unless you meant that the @Override should be added to GrpcOperationsStub, in which case, done.

Also, I'll be adding this to the GAPIC generation, so this will be consistent with a future generated surface.

elharo · 2021-01-28T23:08:50Z

gax-grpc/src/main/java/com/google/longrunning/stub/OperationsStubSettings.java

@@ -361,6 +379,11 @@ public Builder applyToAllUnaryMethods(
      return deleteOperationSettings;
    }

+    /** Returns the builder for the settings used for calls to waitOperation. */
+    public UnaryCallSettings.Builder<WaitOperationRequest, Operation> waitOperationSettings() {


Same as above.

elharo · 2021-01-28T23:09:25Z

gax-grpc/src/test/java/com/google/longrunning/MockOperationsImpl.java

+  public void waitOperation(
+      WaitOperationRequest request, StreamObserver<Operation> responseObserver) {
+    Object response = responses.remove();
+    if (response instanceof Operation) {


This is very strange. The response can have multiple types?

Copied verbatim from the standard generated GAPIC library for the LRO proto, as are the methods above.

elharo · 2021-01-28T23:10:07Z

gax-grpc/src/test/java/com/google/longrunning/OperationsClientTest.java

+  @SuppressWarnings("all")
+  public void waitOperationTest() {
+    String name2 = "name2-1052831874";
+    boolean done = true;


Would prefer to keep this as close to the auto-generated code as possible, so that manual, semantic-impacting changes can be easily identified if this LRO GAPIC ever needs to be regenerated.

elharo · 2021-02-10T13:14:04Z

gax-grpc/src/main/java/com/google/longrunning/OperationsClient.java

+   * }
+   * </code></pre>
+   *
+   * @param request The request object containing all of the parameters for the API call.


The --> the
no period
per Oracle JavaDoc guidelines

elharo · 2021-02-10T13:15:43Z

gax-grpc/src/main/java/com/google/longrunning/OperationsSettings.java

@@ -72,6 +72,11 @@
    return ((OperationsStubSettings) getStubSettings()).deleteOperationSettings();
  }

+  /** Returns the object with the settings used for calls to waitOperation. */
+  public UnaryCallSettings<WaitOperationRequest, Operation> waitOperationSettings() {


Autogenerated or no., I still think a test helps here. If the autogenerated code chages in a way that causes the test to fail, the test will make that apparent.

elharo · 2021-02-10T13:16:02Z

gax-grpc/src/main/java/com/google/longrunning/OperationsSettings.java

@@ -170,6 +175,11 @@ public Builder applyToAllUnaryMethods(
      return getStubSettingsBuilder().deleteOperationSettings();
    }

+    /** Returns the builder for the settings used for calls to deleteOperation. */
+    public UnaryCallSettings.Builder<WaitOperationRequest, Operation> waitOperationSettings() {


I still think there should be a test of this method.

elharo · 2021-02-10T13:17:45Z

gax-grpc/src/test/java/com/google/longrunning/MockOperationsImpl.java

+    } else if (response instanceof Exception) {
+      responseObserver.onError((Exception) response);
+    } else {
+      responseObserver.onError(new IllegalArgumentException("Unrecognized response type"));


include the type the in the message here to ease debugging

Done, will add to the microgenerator as well.

vam-google

LGTM, but I'm wondering how the code was created for this PR (generated, manually or generated+manually).

vam-google · 2021-02-17T18:06:54Z

gax-grpc/src/main/java/com/google/longrunning/stub/GrpcOperationsStub.java

@@ -97,6 +98,15 @@
                  ProtoUtils.marshaller(DeleteOperationRequest.getDefaultInstance()))
              .setResponseMarshaller(ProtoUtils.marshaller(Empty.getDefaultInstance()))
              .build();
+  private static final MethodDescriptor<WaitOperationRequest, Operation>


A question about all of the changes in this PR: are these chagnes were added manually or generated, or some sort of "generated and then manually postprocessed"?

miraleung and others added 5 commits January 28, 2021 11:55

fix(lro): Add Operation name to get, list requests

eef393e

Merge branch 'master' into fix/lro-headers

b714927

fix: add name header to {Cancel,Delete}Operation

7766dd5

Merge branch 'fix/lro-headers' of github.com:googleapis/gax-java into…

989e540

… fix/lro-headers

feat(operations): Add WaitOperation API surface

d5c490f

miraleung requested a review from vam-google January 28, 2021 22:49

miraleung requested review from a team as code owners January 28, 2021 22:49

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 28, 2021

elharo reviewed Jan 28, 2021

View reviewed changes

miraleung and others added 5 commits February 1, 2021 12:26

fix: update OperationsClient comments

e065dea

fix: Add override annotations to GrpcOperationsStub

02dea8a

fix: update comment in OperationsClient

aba567a

Merge branch 'master' into feat/lro-wait

badd77c

Merge branch 'master' into feat/lro-wait

ad84f25

elharo reviewed Feb 10, 2021

View reviewed changes

miraleung added 2 commits February 12, 2021 11:58

fix: comment cleanup

acfcbb5

fix: MockOperationsImpl error message

a1d113f

elharo approved these changes Feb 16, 2021

View reviewed changes

vam-google approved these changes Feb 17, 2021

View reviewed changes

Merge branch 'master' into feat/lro-wait

a82b67e

miraleung merged commit 68761a7 into master Feb 17, 2021

miraleung deleted the feat/lro-wait branch February 17, 2021 21:02

miraleung mentioned this pull request Feb 8, 2023

[gax-java] Add tests for OperationSettings googleapis/sdk-platform-java#1316

Open

feat(operations): Add WaitOperation API surface [gax-java] #1284

feat(operations): Add WaitOperation API surface [gax-java] #1284

Conversation

miraleung commented Jan 28, 2021

codecov bot commented Jan 28, 2021 • edited

Codecov Report

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

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

elharo Feb 10, 2021 • edited

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

elharo Feb 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2021 •

edited

elharo Feb 10, 2021 •

edited

elharo Feb 10, 2021 •

edited