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 @@ -192,7 +192,7 @@ public final Operation getOperation(String name) {
* }
* </code></pre>
*
* @param request The request object containing all of the parameters for the API call.
* @param request the request object containing all of the parameters for the API call
* @throws com.google.api.gax.rpc.ApiException if the remote call fails
*/
private final Operation getOperation(GetOperationRequest request) {
Expand Down Expand Up @@ -502,6 +502,61 @@ public final UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallabl
return stub.deleteOperationCallable();
}

/**
* Waits until the specified long-running operation is done or reaches at most a specified
* 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 until the specified long-running operation is done or reaches at most a specified
* 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 @@ -68,6 +68,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 @@ -166,6 +171,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 @@ -46,6 +46,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 io.grpc.MethodDescriptor;
import io.grpc.protobuf.ProtoUtils;
Expand Down Expand Up @@ -97,6 +98,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 +116,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 @@ -199,6 +210,19 @@ public Map<String, String> extract(DeleteOperationRequest request) {
}
})
.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 =
callableFactory.createUnaryCallable(
Expand All @@ -215,31 +239,44 @@ public Map<String, String> extract(DeleteOperationRequest request) {
this.deleteOperationCallable =
callableFactory.createUnaryCallable(
deleteOperationTransportSettings, settings.deleteOperationSettings(), clientContext);
this.waitOperationCallable =
callableFactory.createUnaryCallable(
waitOperationTransportSettings, settings.waitOperationSettings(), clientContext);

backgroundResources = new BackgroundResourceAggregation(clientContext.getBackgroundResources());
}

@Override
public UnaryCallable<GetOperationRequest, Operation> getOperationCallable() {
return getOperationCallable;
}

@Override
public UnaryCallable<ListOperationsRequest, ListOperationsPagedResponse>
listOperationsPagedCallable() {
return listOperationsPagedCallable;
}

@Override
public UnaryCallable<ListOperationsRequest, ListOperationsResponse> listOperationsCallable() {
return listOperationsCallable;
}

@Override
public UnaryCallable<CancelOperationRequest, Empty> cancelOperationCallable() {
return cancelOperationCallable;
}

@Override
public UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallable() {
return deleteOperationCallable;
}

@Override
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;

/**
Expand Down Expand Up @@ -71,6 +72,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 org.threeten.bp.Duration;
Expand All @@ -75,6 +76,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 @@ -98,6 +100,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 @@ -151,6 +158,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 @@ -215,6 +223,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 @@ -265,6 +274,8 @@ protected Builder(ClientContext clientContext) {

deleteOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder();

waitOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder();

unaryMethodSettingsBuilders =
ImmutableList.<UnaryCallSettings.Builder<?, ?>>of(
getOperationSettings,
Expand Down Expand Up @@ -302,6 +313,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 @@ -312,13 +328,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 @@ -358,6 +376,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.

}
}
}
Expand Up @@ -38,6 +38,7 @@
import com.google.api.gax.rpc.InvalidArgumentException;
import com.google.common.collect.Lists;
import com.google.protobuf.AbstractMessage;
import com.google.protobuf.Duration;
import com.google.protobuf.Empty;
import io.grpc.Status;
import io.grpc.StatusRuntimeException;
Expand Down Expand Up @@ -235,4 +236,46 @@ public void deleteOperationExceptionTest() throws Exception {
// Expected exception
}
}

@Test
@SuppressWarnings("all")
public void waitOperationTest() {
String name2 = "name2-1052831874";
boolean done = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

inline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Operation expectedResponse = Operation.newBuilder().setName(name2).setDone(done).build();
mockOperations.addResponse(expectedResponse);

String name = "name3373707";
Duration timeout = Duration.newBuilder().setSeconds(5).build();
WaitOperationRequest request =
WaitOperationRequest.newBuilder().setName(name).setTimeout(timeout).build();

Operation actualResponse = client.waitOperation(request);
Assert.assertEquals(expectedResponse, actualResponse);

List<AbstractMessage> actualRequests = mockOperations.getRequests();
Assert.assertEquals(1, actualRequests.size());
WaitOperationRequest actualRequest = (WaitOperationRequest) actualRequests.get(0);

Assert.assertEquals(name, actualRequest.getName());
}

@Test
@SuppressWarnings("all")
public void waitOperationExceptionTest() throws Exception {
StatusRuntimeException exception = new StatusRuntimeException(Status.INVALID_ARGUMENT);
mockOperations.addException(exception);

try {
String name = "name3373707";
Duration timeout = Duration.newBuilder().setSeconds(5).build();
WaitOperationRequest request =
WaitOperationRequest.newBuilder().setName(name).setTimeout(timeout).build();

client.waitOperation(request);
Assert.fail("No exception raised");
} catch (InvalidArgumentException e) {
// Expected exception
}
}
}