-
Notifications
You must be signed in to change notification settings - Fork 106
feat(operations): Add WaitOperation API surface [gax-java] #1284
Changes from all commits
eef393e
b714927
7766dd5
989e540
d5c490f
e065dea
02dea8a
aba567a
badd77c
ad84f25
acfcbb5
a1d113f
a82b67e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
return ((OperationsStubSettings) getStubSettings()).waitOperationSettings(); | ||
} | ||
|
||
public static final OperationsSettings create(OperationsStubSettings stub) throws IOException { | ||
return new OperationsSettings.Builder(stub.toBuilder()).build(); | ||
} | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think there should be a test of this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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; | ||
|
||
|
@@ -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( | ||
|
@@ -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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
/** | ||
|
@@ -71,6 +72,10 @@ public UnaryCallable<DeleteOperationRequest, Empty> deleteOperationCallable() { | |
throw new UnsupportedOperationException("Not implemented: deleteOperationCallable()"); | ||
} | ||
|
||
public UnaryCallable<WaitOperationRequest, Operation> waitOperationCallable() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's a corresponding method in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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() { | ||
|
@@ -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() | ||
|
@@ -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> | ||
|
@@ -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; | ||
|
@@ -265,6 +274,8 @@ protected Builder(ClientContext clientContext) { | |
|
||
deleteOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); | ||
|
||
waitOperationSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); | ||
|
||
unaryMethodSettingsBuilders = | ||
ImmutableList.<UnaryCallSettings.Builder<?, ?>>of( | ||
getOperationSettings, | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,4 +130,25 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very strange. The response can have multiple types? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
String.format( | ||
"Unrecognized response type %s, expected %s or %s", | ||
response.getClass().getName(), | ||
Operation.class.getName(), | ||
Exception.class.getName()))); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -235,4 +236,46 @@ public void deleteOperationExceptionTest() throws Exception { | |
// Expected exception | ||
} | ||
} | ||
|
||
@Test | ||
@SuppressWarnings("all") | ||
public void waitOperationTest() { | ||
String name2 = "name2-1052831874"; | ||
boolean done = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inline There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks testable
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.