Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

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

Merged
merged 13 commits into from Feb 17, 2021
Merged
Expand Up @@ -519,6 +519,61 @@ public final UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallabl
return stub.deleteOperationCallable();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* timeout, returning the latest state. If the operation is already done, the latest state is
* immediately returned. If the timeout specified is greater than the default HTTP/RPC timeout,
* the HTTP/RPC timeout is used. If the server does not support this method, it returns
* `google.rpc.Code.UNIMPLEMENTED`. Note that this method is on a best-effort basis. It may return
* the latest state before the specified timeout (including immediately), meaning even an
* immediate response is no guarantee that the operation is done.
*
* <p>Sample code:
*
* <pre><code>
* try (OperationsClient operationsClient = OperationsClient.create()) {
* String name = "";
* WaitOperationRequest request = WaitOperationRequest.newBuilder()
* .setName(name)
* .setTimeout(Duration.ofMillis(100))
* .build();
* Operation response = operationsClient.waitOperation(request);
* }
* </code></pre>
*
* @param request The request object containing all of the parameters for the API call.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --> the
no period

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to push?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --> the
no period
per Oracle JavaDoc guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @throws com.google.api.gax.rpc.ApiException if the remote call fails
*/
public final Operation waitOperation(WaitOperationRequest request) {
return waitOperationCallable().call(request);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

* timeout, returning the latest state. If the operation is already done, the latest state is
* immediately returned. If the timeout specified is greater than the default HTTP/RPC timeout,
* the HTTP/RPC timeout is used. If the server does not support this method, it returns
* `google.rpc.Code.UNIMPLEMENTED`. Note that this method is on a best-effort basis. It may return
* the latest state before the specified timeout (including immediately), meaning even an
* immediate response is no guarantee that the operation is done.
*
* <p>Sample code:
*
* <pre><code>
* try (OperationsClient operationsClient = OperationsClient.create()) {
* String name = "";
* WaitOperationRequest request = WaitOperationRequest.newBuilder()
* .setName(name)
* .setTimeout(Duration.ofMillis(100))
* .build();
* ApiFuture&lt;Operation&gt; future = operationsClient.waitOperationCallable().futureCall(request);
* }
* </code></pre>
*/
public final UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable() {
return stub.waitOperationCallable();
}

@Override
public final void close() {
stub.close();
Expand Down
Expand Up @@ -72,6 +72,11 @@ public UnaryCallSettings<DeleteOperationRequest, Empty> deleteOperationSettings(
return ((OperationsStubSettings) getStubSettings()).deleteOperationSettings();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks testable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping. This is a critical issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ((OperationsStubSettings) getStubSettings()).waitOperationSettings();
}

public static final OperationsSettings create(OperationsStubSettings stub) throws IOException {
return new OperationsSettings.Builder(stub.toBuilder()).build();
}
Expand Down Expand Up @@ -170,6 +175,11 @@ public UnaryCallSettings.Builder<DeleteOperationRequest, Empty> deleteOperationS
return getStubSettingsBuilder().deleteOperationSettings();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor

@elharo elharo Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return getStubSettingsBuilder().waitOperationSettings();
}

@Override
public OperationsSettings build() throws IOException {
return new OperationsSettings(this);
Expand Down
Expand Up @@ -37,17 +37,21 @@
import com.google.api.gax.grpc.GrpcCallSettings;
import com.google.api.gax.grpc.GrpcStubCallableFactory;
import com.google.api.gax.rpc.ClientContext;
import com.google.api.gax.rpc.RequestParamsExtractor;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.common.collect.ImmutableMap;
import com.google.longrunning.CancelOperationRequest;
import com.google.longrunning.DeleteOperationRequest;
import com.google.longrunning.GetOperationRequest;
import com.google.longrunning.ListOperationsRequest;
import com.google.longrunning.ListOperationsResponse;
import com.google.longrunning.Operation;
import com.google.longrunning.WaitOperationRequest;
import com.google.protobuf.Empty;
import io.grpc.MethodDescriptor;
import io.grpc.protobuf.ProtoUtils;
import java.io.IOException;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import javax.annotation.Generated;

Expand Down Expand Up @@ -97,6 +101,15 @@ public class GrpcOperationsStub extends OperationsStub {
ProtoUtils.marshaller(DeleteOperationRequest.getDefaultInstance()))
.setResponseMarshaller(ProtoUtils.marshaller(Empty.getDefaultInstance()))
.build();
private static final MethodDescriptor<WaitOperationRequest, Operation>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

waitOperationMethodDescriptor =
MethodDescriptor.<WaitOperationRequest, Operation>newBuilder()
.setType(MethodDescriptor.MethodType.UNARY)
.setFullMethodName("google.longrunning.Operations/WaitOperation")
.setRequestMarshaller(
ProtoUtils.marshaller(WaitOperationRequest.getDefaultInstance()))
.setResponseMarshaller(ProtoUtils.marshaller(Operation.getDefaultInstance()))
.build();

private final BackgroundResource backgroundResources;

Expand All @@ -106,6 +119,7 @@ public class GrpcOperationsStub extends OperationsStub {
listOperationsPagedCallable;
private final UnaryCallable<CancelOperationRequest, Empty> cancelOperationCallable;
private final UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallable;
private final UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable;

private final GrpcStubCallableFactory callableFactory;

Expand Down Expand Up @@ -149,19 +163,68 @@ protected GrpcOperationsStub(
GrpcCallSettings<GetOperationRequest, Operation> getOperationTransportSettings =
GrpcCallSettings.<GetOperationRequest, Operation>newBuilder()
.setMethodDescriptor(getOperationMethodDescriptor)
.setParamsExtractor(
new RequestParamsExtractor<GetOperationRequest>() {
@Override
public Map<String, String> extract(GetOperationRequest request) {
ImmutableMap.Builder<String, String> params = ImmutableMap.builder();
params.put("name", String.valueOf(request.getName()));
return params.build();
}
})
.build();
GrpcCallSettings<ListOperationsRequest, ListOperationsResponse>
listOperationsTransportSettings =
GrpcCallSettings.<ListOperationsRequest, ListOperationsResponse>newBuilder()
.setMethodDescriptor(listOperationsMethodDescriptor)
.setParamsExtractor(
new RequestParamsExtractor<ListOperationsRequest>() {
@Override
public Map<String, String> extract(ListOperationsRequest request) {
ImmutableMap.Builder<String, String> params = ImmutableMap.builder();
params.put("name", String.valueOf(request.getName()));
return params.build();
}
})
.build();
GrpcCallSettings<CancelOperationRequest, Empty> cancelOperationTransportSettings =
GrpcCallSettings.<CancelOperationRequest, Empty>newBuilder()
.setMethodDescriptor(cancelOperationMethodDescriptor)
.setParamsExtractor(
new RequestParamsExtractor<CancelOperationRequest>() {
@Override
public Map<String, String> extract(CancelOperationRequest request) {
ImmutableMap.Builder<String, String> params = ImmutableMap.builder();
params.put("name", String.valueOf(request.getName()));
return params.build();
}
})
.build();
GrpcCallSettings<DeleteOperationRequest, Empty> deleteOperationTransportSettings =
GrpcCallSettings.<DeleteOperationRequest, Empty>newBuilder()
.setMethodDescriptor(deleteOperationMethodDescriptor)
.setParamsExtractor(
new RequestParamsExtractor<DeleteOperationRequest>() {
@Override
public Map<String, String> extract(DeleteOperationRequest request) {
ImmutableMap.Builder<String, String> params = ImmutableMap.builder();
params.put("name", String.valueOf(request.getName()));
return params.build();
}
})
.build();
GrpcCallSettings<WaitOperationRequest, Operation> waitOperationTransportSettings =
GrpcCallSettings.<WaitOperationRequest, Operation>newBuilder()
.setMethodDescriptor(waitOperationMethodDescriptor)
.setParamsExtractor(
new RequestParamsExtractor<WaitOperationRequest>() {
@Override
public Map<String, String> extract(WaitOperationRequest request) {
ImmutableMap.Builder<String, String> params = ImmutableMap.builder();
params.put("name", String.valueOf(request.getName()));
return params.build();
}
})
.build();

this.getOperationCallable =
Expand All @@ -179,6 +242,9 @@ protected GrpcOperationsStub(
this.deleteOperationCallable =
callableFactory.createUnaryCallable(
deleteOperationTransportSettings, settings.deleteOperationSettings(), clientContext);
this.waitOperationCallable =
callableFactory.createUnaryCallable(
waitOperationTransportSettings, settings.waitOperationSettings(), clientContext);

backgroundResources = new BackgroundResourceAggregation(clientContext.getBackgroundResources());
}
Expand All @@ -204,6 +270,10 @@ public UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallable() {
return deleteOperationCallable;
}

public UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable() {
return waitOperationCallable;
}

@Override
public final void close() {
shutdown();
Expand Down
Expand Up @@ -40,6 +40,7 @@
import com.google.longrunning.ListOperationsRequest;
import com.google.longrunning.ListOperationsResponse;
import com.google.longrunning.Operation;
import com.google.longrunning.WaitOperationRequest;
import com.google.protobuf.Empty;
import javax.annotation.Generated;

Expand Down Expand Up @@ -74,6 +75,10 @@ public UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallable() {
throw new UnsupportedOperationException("Not implemented: deleteOperationCallable()");
}

public UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new UnsupportedOperationException("Not implemented: waitOperationCallable()");
}

@Override
public abstract void close();
}
Expand Up @@ -61,6 +61,7 @@
import com.google.longrunning.ListOperationsRequest;
import com.google.longrunning.ListOperationsResponse;
import com.google.longrunning.Operation;
import com.google.longrunning.WaitOperationRequest;
import com.google.protobuf.Empty;
import java.io.IOException;
import javax.annotation.Generated;
Expand All @@ -78,6 +79,7 @@ public class OperationsStubSettings extends StubSettings<OperationsStubSettings>
listOperationsSettings;
private final UnaryCallSettings<CancelOperationRequest, Empty> cancelOperationSettings;
private final UnaryCallSettings<DeleteOperationRequest, Empty> deleteOperationSettings;
private final UnaryCallSettings<WaitOperationRequest, Operation> waitOperationSettings;

/** Returns the object with the settings used for calls to getOperation. */
public UnaryCallSettings<GetOperationRequest, Operation> getOperationSettings() {
Expand All @@ -101,6 +103,11 @@ public UnaryCallSettings<DeleteOperationRequest, Empty> deleteOperationSettings(
return deleteOperationSettings;
}

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

@BetaApi("A restructuring of stub classes is planned, so this may break in the future")
public OperationsStub createStub() throws IOException {
if (getTransportChannelProvider()
Expand Down Expand Up @@ -154,6 +161,7 @@ protected OperationsStubSettings(Builder settingsBuilder) throws IOException {
listOperationsSettings = settingsBuilder.listOperationsSettings().build();
cancelOperationSettings = settingsBuilder.cancelOperationSettings().build();
deleteOperationSettings = settingsBuilder.deleteOperationSettings().build();
waitOperationSettings = settingsBuilder.waitOperationSettings().build();
}

private static final PagedListDescriptor<ListOperationsRequest, ListOperationsResponse, Operation>
Expand Down Expand Up @@ -218,6 +226,7 @@ public static class Builder extends StubSettings.Builder<OperationsStubSettings,
listOperationsSettings;
private final UnaryCallSettings.Builder<CancelOperationRequest, Empty> cancelOperationSettings;
private final UnaryCallSettings.Builder<DeleteOperationRequest, Empty> deleteOperationSettings;
private final UnaryCallSettings.Builder<WaitOperationRequest, Operation> waitOperationSettings;

private static final ImmutableMap<String, ImmutableSet<StatusCode.Code>>
RETRYABLE_CODE_DEFINITIONS;
Expand Down Expand Up @@ -268,6 +277,8 @@ protected Builder(ClientContext clientContext) {

deleteOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder();

waitOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder();

unaryMethodSettingsBuilders =
ImmutableList.<UnaryCallSettings.Builder<?, ?>>of(
getOperationSettings,
Expand Down Expand Up @@ -305,6 +316,11 @@ private static Builder initDefaults(Builder builder) {
.setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("idempotent"))
.setRetrySettings(RETRY_PARAM_DEFINITIONS.get("default"));

builder
.waitOperationSettings()
.setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("idempotent"))
.setRetrySettings(RETRY_PARAM_DEFINITIONS.get("default"));

return builder;
}

Expand All @@ -315,13 +331,15 @@ protected Builder(OperationsStubSettings settings) {
listOperationsSettings = settings.listOperationsSettings.toBuilder();
cancelOperationSettings = settings.cancelOperationSettings.toBuilder();
deleteOperationSettings = settings.deleteOperationSettings.toBuilder();
waitOperationSettings = settings.waitOperationSettings.toBuilder();

unaryMethodSettingsBuilders =
ImmutableList.<UnaryCallSettings.Builder<?, ?>>of(
getOperationSettings,
listOperationsSettings,
cancelOperationSettings,
deleteOperationSettings);
deleteOperationSettings,
waitOperationSettings);
}

/**
Expand Down Expand Up @@ -361,6 +379,11 @@ public UnaryCallSettings.Builder<DeleteOperationRequest, Empty> deleteOperationS
return deleteOperationSettings;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

return waitOperationSettings;
}

@Override
public OperationsStubSettings build() throws IOException {
return new OperationsStubSettings(this);
Expand Down
Expand Up @@ -130,4 +130,19 @@ public void cancelOperation(
responseObserver.onError(new IllegalArgumentException("Unrecognized response type"));
}
}

@Override
public void waitOperation(
WaitOperationRequest request, StreamObserver<Operation> responseObserver) {
Object response = responses.remove();
if (response instanceof Operation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

requests.add(request);
responseObserver.onNext((Operation) response);
responseObserver.onCompleted();
} else if (response instanceof Exception) {
responseObserver.onError((Exception) response);
} else {
responseObserver.onError(new IllegalArgumentException("Unrecognized response type"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include the type the in the message here to ease debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, will add to the microgenerator as well.

}
}
}