Add LRO support #42

michaelbausor · 2016-10-31T17:20:31Z

This is a possible OperationResponse object for use with LRO in PHP. Some points to note:

I'm not sure if we want to support promises, so I have commented them as optional. We can add support for them later in a non-breaking way
Calling getResult() returns null when the operation is not complete, and throws when it has completed with an error. Maybe it should return null when completed with an error?
A consequence of getResult() throwing is that pollUntilComplete() does not have a return value (it can't return getResult() because that might throw), which complicates the simple case

jdpedrie · 2016-10-31T17:29:14Z

Thanks for this, Michael! I'm excited to get my hands on it.

@dwsupplee and I agree that if an LRO completes with an error, it should not throw an exception. Instead, the response should be returned, and left to the implementor to deal with.

If there is an error in any of the requests, (i.e. an invalid argument or a server error), those exceptions should certainly still be thrown.

michaelbausor · 2016-10-31T17:45:55Z

OK no worries. If we are returning either the response or error, do you prefer:

Just return either the unpacked response, or the Status object containing the error, and let the user figure it out
Return an array, which would look like [true, response] for success or [false, status] for error
Return an array, which would look like [Status.code.OK, response] for success or [Status.code, status] for error

The reason to prefer either the second or third option over the first is to provide an easy way for people to check for success or failure, instead of having to interrogate the return type. Alternatively, we could provide helpers on the OperationResponse object, but this feels more awkward to me.

Thanks for having a look so quickly!

jdpedrie · 2016-10-31T18:28:44Z

I should have been more careful in my previous comment. When I said "response", I was thinking in HTTP terms (i.e. response means the entire result returned from a call). I'd like to have the entire Operation available to manage as I see fit in a given situation. So I could check whether Operation.response or Operation.error were set and handle things as needed in either case. Apologies!

michaelbausor · 2016-10-31T18:44:25Z

No worries, you are right that we should make the proto object available. I had intended to do that but missed that method :-( There is now an added method to get the operation proto object.

That still leaves the question of what to return from the getResult() method, which I think is a useful helper so that it isn't necessary to get the operation proto in most cases.

src/OperationResponse.php

+// Simple case
+$op = $sampleApi->longRunningRpc();
+// ... do stuff ...
+$op->pollUntilComplete();


src/OperationResponse.php

+            // TODO: use poll settings
+            sleep(1);
+            $this->refresh();
+        }


jdpedrie · 2016-11-01T16:12:07Z

@michaelbausor WDYT about having getResult() and getError() methods, both of which are null when done is false. Once complete, on success, getResult() will return the response and getError() will remain null. On error, vice versa.

I think explicitly choosing the result or the error makes it easier to prevent errors in mishandling an unexpected result (i.e. if you code around what a success should look like and instead get an error, will your code break?).

$op = $sampleApi->longRunningRpc();
$op->pollUntilComplete();

if (!is_null($op->getError())) {
    // handle errors
}

return $op->getResult();

michaelbausor · 2016-11-01T16:37:58Z

I think that sounds sensible. That makes the OperationResponse object very similar in structure to the proto object, will helpers for refresh(), cancel(), pollUntilComplete() and using promises.

dwsupplee · 2016-11-01T17:16:29Z

@michaelbausor do we have the ability to make async calls with gRPC yet? If yes, it would be great if we could set some time aside to come up with a good plan for how to handle promises. For example, I have a WIP branch right now that introduces Async versions of all our veneer methods that trigger network requests by utilizing guzzle's promises library. If we could match a similar style in the generated code that would be really awesome.

michaelbausor · 2016-11-01T17:51:48Z

Not right now through Veneer toolkit, but I am looking into what would be required to add that support. Definitely it would be great to pick a time to discuss how we want to handle promises more generally - how about Thursday or Friday this week some time?

dwsupplee · 2016-11-01T18:35:50Z

Friday should work for me!

michaelbausor · 2016-11-01T18:54:13Z

Following up re status - it seems that the latest is your open issue in gRPC (grpc/grpc#6654), which hasn't been given a priority yet. So we aren't able to support async over gRPC at this time. But it would still be worth having the discussion so that we are on the same page. Friday sounds good, I have created an event at 10:30 PST as a placeholder, please edit or ping me if that doesn't work for you.

codecov-io · 2016-11-21T23:35:13Z

Current coverage is 89.78% (diff: 88.15%)

Merging #42 into master will increase coverage by 0.31%

@@             master        #42   diff @@
==========================================
  Files            16         18     +2   
  Lines          1130       1341   +211   
  Methods         123        147    +24   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1011       1204   +193   
- Misses          119        137    +18   
  Partials          0          0

Powered by Codecov. Last update b4930f6...545efa8

Generate operations API, update operations wrapper Update operation and callable

michaelbausor · 2016-12-14T01:22:24Z

I have updated the proposed LRO surface, PTAL when you have a chance. It uses getResult() and getError() as @jdpedrie suggested above.

pollUntilComplete() accepts an optional callback, but I am not sure if this is adding very much value - perhaps it should be removed.

cc @bshaffer

michaelbausor · 2016-12-20T22:00:54Z

@garrettjonesgoogle @shinfan PTAL for review before merging

src/Longrunning/OperationsClient.php

+    /**
+     * The default address of the service.
+     */
+    const SERVICE_ADDRESS = 'longrunning.googleapis.com';


src/Longrunning/OperationsClient.php

     *
-     *     @type string $serviceAddress The domain name of the API remote host.
-     *                                  Default 'longrunning.googleapis.com'.
+     *     @type string $serviceAddress Required. The domain name of the API remote host.


dwsupplee

This is looking nice overall! Just a few comments and things I am curious on.

composer.json

@@ -17,7 +17,8 @@
    },
    "autoload": {
        "psr-4": {
-          "Google\\GAX\\":"src/"
+          "Google\\GAX\\":"src/",
+          "Google\\Longrunning\\":"src/Longrunning/"


src/Longrunning/OperationsClient.php

+ * interface to receive the real response asynchronously by polling the
+ * operation resource, or pass the operation resource to another API (such as
+ * Google Cloud Pub/Sub API) to receive the response.  Any API service that
+ * returns long-running operations should implement the `Operations` interface


src/Longrunning/OperationsClient.php

+ * ```
+ *
+ * Many parameters require resource names to be formatted in a particular way. To assist
+ * with these names, this class includes a format method for each type of name, and additionally


src/Longrunning/OperationsClient.php

+ * Service Description: Manages long-running operations with an API service.
+ *
+ * When an API method normally takes long time to complete, it can be designed
+ * to return [Operation][google.longrunning.Operation] to the client, and the client can use this


src/Longrunning/OperationsClient.php

+     * ```
+     *
+     * @param string $name         The name of the operation collection.
+     * @param string $filter       The standard list filter.


src/OperationResponse.php

+     * Return true if the operation completed successfully, otherwise return
+     * false.
+     *
+     * The $pollSettings optional argument can be used to control the polling loop.


src/OperationResponse.php

+    }
+
+    /**
+     * @return \google\longrunning\Operation The last Operation object received from the server.


src/OperationResponse.php

+     * Starts asynchronous cancellation on a long-running operation. The server
+     * makes a best effort to cancel the operation, but success is not
+     * guaranteed. If the server doesn't support this method, it will throw an
+     * ApiException with code \google\rpc\Code::UNIMPLEMENTED. Clients can continue


src/OperationResponse.php

+
+        $anyResponse = $this->lastProtoResponse->getResponse();
+        if (is_null($this->operationReturnType)) {
+            return $anyResponse;


src/OperationResponse.php

+     * @param array $options {
+     *                       Options for configuring the polling behaviour.
+     *
+     *     @type float $pollingIntervalSeconds The polling interval to use, in seconds.


michaelbausor

Thanks @dwsupplee !

I have made changes and/or commented, PTAL

src/OperationResponse.php

+     * @param array $options {
+     *                       Options for configuring the polling behaviour.
+     *
+     *     @type float $pollingIntervalSeconds The polling interval to use, in seconds.


dwsupplee · 2016-12-23T14:04:44Z

The updates look great, nice work!

src/OperationResponse.php

+     * OperationResponse constructor.
+     *
+     * @param string $operationName
+     * @param \Google\Longrunning\OperationsClient $operationsClient


src/OperationResponse.php

+    }
+
+    /**
+     * @return \Google\Longrunning\OperationsClient The OperationsClient object used to make


src/Longrunning/OperationsClient.php

+ * backwards compatibility.
+ */
+
+namespace Google\GAX\Longrunning;


- Change LRO namespace to camel case - Add license

michaelbausor · 2016-12-29T23:46:26Z

PTAL

michaelbausor assigned shinfan and garrettjonesgoogle Oct 31, 2016

shinfan reviewed Oct 31, 2016

View reviewed changes

src/OperationResponse.php

// Simple case

$op = $sampleApi->longRunningRpc();

// ... do stuff ...

$op->pollUntilComplete();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

shinfan reviewed Oct 31, 2016

View reviewed changes

src/OperationResponse.php

// TODO: use poll settings

sleep(1);

$this->refresh();

}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

michaelbausor added 10 commits December 13, 2016 15:28

DO NOT MERGE: LRO sample

547fbfd

Update with getProtoResponse and other samples

fa54b7c

LRO updates

4f66cb4

Generate operations API, update operations wrapper Update operation and callable

remove lro sample

8c3e427

Regenerate operations API

fa113f8

Start adding polling

bb333ba

Restructure, add comments

68bfcc4

Update operations api and response object

53198df

Fix syntax error

e4bffe5

Fix syntax error

24d8b81

michaelbausor force-pushed the lro branch from e4a5fda to 24d8b81 Compare December 14, 2016 01:15

michaelbausor added 3 commits December 20, 2016 13:36

Add tests

824a64c

Update docs, composer

7f6c838

remove handler from pollUntilComplete

7c899c5

Doc fixes

ff8a734

michaelbausor changed the title ~~DO NOT MERGE: LRO sample~~ Add LRO support Dec 20, 2016

michaelbausor mentioned this pull request Dec 20, 2016

Add LRO support for PHP googleapis/gapic-generator#875

Merged

garrettjonesgoogle reviewed Dec 20, 2016

View reviewed changes

src/Longrunning/OperationsClient.php

/**

* The default address of the service.

*/

const SERVICE_ADDRESS = 'longrunning.googleapis.com';

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

michaelbausor added 2 commits December 20, 2016 16:45

Regenerate OperationsClient without scopes and serviceAddress

3cd9959

Update tests to set serviceAddress and scopes in OperationsClient

ec982e2

garrettjonesgoogle reviewed Dec 21, 2016

View reviewed changes

michaelbausor mentioned this pull request Dec 22, 2016

DO NOT MERGE: Unit test generation michaelbausor/gax-php#2

Open

dwsupplee reviewed Dec 22, 2016

View reviewed changes

michaelbausor added 2 commits December 22, 2016 13:19

Address PR comments

59c746d

Add max duration setting to pollUntilComplete

a12e50f

michaelbausor commented Dec 22, 2016

View reviewed changes

src/OperationResponse.php

* @param array $options {

* Options for configuring the polling behaviour.

*

* @type float $pollingIntervalSeconds The polling interval to use, in seconds.

This comment was marked as spam.

Sign in to view

Add operationSucceeded and operationFailed functions

3eef214

michaelbausor added 2 commits December 27, 2016 09:40

Update OperationsClient namespace

774079a

Update namespace in import

c84d412

bshaffer reviewed Dec 27, 2016

View reviewed changes

src/OperationResponse.php

* OperationResponse constructor.

*

* @param string $operationName

* @param \Google\Longrunning\OperationsClient $operationsClient

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

bshaffer reviewed Dec 27, 2016

View reviewed changes

src/OperationResponse.php

}

/**

* @return \Google\Longrunning\OperationsClient The OperationsClient object used to make

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

bshaffer reviewed Dec 27, 2016

View reviewed changes

src/Longrunning/OperationsClient.php

* backwards compatibility.

*/

namespace Google\GAX\Longrunning;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

michaelbausor added 3 commits December 27, 2016 15:27

Change LRO namespace to camel case

aceb645

- Change LRO namespace to camel case - Add license

Camel case Longrunning namespace

497320b

Edit OperationsClient comments

545efa8

dwsupplee approved these changes Dec 30, 2016

View reviewed changes

michaelbausor merged commit 0189f6f into googleapis:master Jan 3, 2017

Add LRO support #42

Add LRO support #42

Conversation

michaelbausor commented Oct 31, 2016

jdpedrie commented Oct 31, 2016 • edited Loading

michaelbausor commented Oct 31, 2016

jdpedrie commented Oct 31, 2016 • edited Loading

michaelbausor commented Oct 31, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jdpedrie commented Nov 1, 2016

michaelbausor commented Nov 1, 2016

dwsupplee commented Nov 1, 2016

michaelbausor commented Nov 1, 2016

dwsupplee commented Nov 1, 2016

michaelbausor commented Nov 1, 2016

codecov-io commented Nov 21, 2016 • edited Loading

Current coverage is 89.78% (diff: 88.15%)

michaelbausor commented Dec 14, 2016

michaelbausor commented Dec 20, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dwsupplee left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

michaelbausor left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

dwsupplee commented Dec 23, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

michaelbausor commented Dec 29, 2016

jdpedrie commented Oct 31, 2016 •

edited

Loading

jdpedrie commented Oct 31, 2016 •

edited

Loading

codecov-io commented Nov 21, 2016 •

edited

Loading