Enable gRPC GCP channel management library for the spanner API #1146

ZhouyihaiDing · 2018-06-29T20:17:17Z

This PR works is used to express the idea for enabling the grpc-gcp library. It depends on PR: GCP library for channel management.

Hi @chingor13 @tmatsuo @bshaffer . I find that there is no extra layer between the user and the generated client like SpannerGapicClient. It seems I need to change the gcp.APIGenerator to regenerate SpannerGapicClient.php. I didn't find where it is generated, can you please give me some suggestions?

My idea is when the user sets ['enable_gcp' => true], Spanner client will read the $api.grpc.config and put the constructed $call_invoker into the $options. The way python does it is when the user calls import grpc_gcp, it will enable the gcp support.

In addition, I am not sure how to run spanner tests locally. I followed what travis did, but there were lot of errors.

Update:
I was wrong because I was looking at the generated SpannerClient.php instead of the real client. I think I can insert the code between the src/SpannerClient.php and the src/V1/SpannerClient.php to avoid involving gax repository.

Waiting for the gRPC 1.13.0 update.

ZhouyihaiDing · 2018-06-29T21:46:15Z

Hi @jdpedrie . I am trying to solve the spanner issue you submitted in gRPC repo by hooking a channel management support via call invoker, which is implement in the link above. I am testing it with zhouyihaiding/grpc-gcp-test composer package.
Spanner client needs to use spanner.grpc.config to create a call invoker with the API \Grpc\Gcp\ApiConfig.

ZhouyihaiDing · 2018-06-29T23:03:06Z

I think I find the place: https://github.com/googleapis/gapic-generator/blob/master/src/main/resources/com/google/api/codegen/php/client_impl.snip#L367. But by doing so, all gapic client will be updated.

ZhouyihaiDing · 2018-06-29T23:12:08Z

Oh I think I use the wrong SpannerClient. It should be the Spanner/src/SpannerClient.php instead of Spanner/src/V1/Spanner.php

jdpedrie · 2018-07-02T13:32:44Z

My apologies for the delayed response @ZhouyihaiDing. I was out of town for the past few days. I'll take a look today!

ZhouyihaiDing · 2018-07-02T16:08:24Z

Thanks, @jdpedrie !
I am testing the implementation downloaded via composer from a test repository if you are interested. https://github.com/ZhouyihaiDing/grpc-gcp-test
Thank you!

dwsupplee · 2018-07-02T16:26:40Z

As a quick note in regards to src/V1/SpannerClient.php (the generated client) vs. src/SpannerClient.php (veneer client) - It actually strikes me as preferable to make the change in the generated client.

If we solve this there, we could get the benefit of this configuration amongst all our components (some of which only have generated clients). Also - the veneer wraps the generated client, so we can satisfy users who use either client.

ZhouyihaiDing · 2018-07-02T16:48:52Z

Thanks! That makes sense. I am going to add more unit tests if there is no $api.grpc.config set.
Maybe we can do it gradually? Spanner first and then all other clients(generated client) because currently only spanner client is more likely to hit the 100 streams bound?

dwsupplee · 2018-07-02T19:17:11Z

Sounds like a solid plan 👍

Spanner/src/Connection/Grpc.php

@@ -152,6 +160,15 @@ public function __construct(array $config = [])
                ? $config['authHttpHandler']
                : null
        );
+
+        if (isset($config['enable-gcp'])) {


ZhouyihaiDing · 2018-07-16T20:07:11Z

Thanks! I am going to use enable-gcp-optimizer.
Since all travis tests have passed, I am going to substitute the test composer library to the real composer library.
Thanks!

Spanner/src/Connection/Grpc.php

@@ -120,6 +122,15 @@ class Grpc implements ConnectionInterface
     */
    private $longRunningGrpcClients;

+    private function enableChonnectionManagement($conf_path)


jdpedrie

Looks ok to me. @dwsupplee can you weigh in as well?

dwsupplee · 2018-08-07T17:06:47Z

Sure thing, will review today

ZhouyihaiDing · 2018-08-08T03:37:04Z

It's not good to let the user sets the flag to enable the grpc-gcp library by themselves. We should handle it inside cloud-php by ourself.

I removed the flag enableGcpOptimizer exposed to the client config. The support is enabled by default now and can be disabled by an environment flag. Later we can use the grpc.spanner.config file to enable/disable the support(like adding a line enable_gcp: true). Since the user doesn't the way we enable/disable the support, it won't affect the user.

Update:
When enable by default, seems all tests are passed according to the travis.

dwsupplee · 2018-08-10T17:39:33Z

@ZhouyihaiDing, I think I may have misunderstood our previous discussion. I thought we were going to tackle this in the spanner generated client first, then figure out a process for the others.

ZhouyihaiDing · 2018-08-10T17:54:45Z

Hi @dwsupplee .

I think it is still the plan. We first enable the gcp library for the spanner client. It is enabled inside cloud-php so the user doesn't need to config it.
The environment variable is only used for cloud-php to enable/disable it so we don't need to remove/add the core code for the first release. When the first release stays stable, we can remove this env var and use "rollback" if something goes wrong in the new lease.

I will also create a same PR to the repo GoogleCloudPlatform/google-cloud-php-spanner after this PR gets merged.

cc: @wenbozhu

ZhouyihaiDing · 2018-08-10T22:21:46Z

Hi @dwsupplee @jdpedrie @chingor13

I think the gcp library is ready to be merged and used in the cloud-php which spanner client will use it first due to the issue #1052.

I modified the PR and removed the flag from the client config. It should only be seens and enabled/disabled by the cloud-php. The environment variable is supposed to be used for the first gcp library release. Later we can remove it and use gcp library all the time. If there are failures caused by the new release, we only need to rollback the library version inside composer.json. If you have some suggestions about it, please let me know.

Can you please take a look at this PR and merge it if there are no other concerns?

dwsupplee

In your comment you mention not having to deal with GAX - is there a reason for this? It seems like working this into the GAPICs themselves seems like the ideal way to move forward. Would you be open to us helping to port this concept over into GAX/the generated clients?

Spanner/src/Connection/Grpc.php

+        // TODO(ddyihai): move this function to GrpcTrait, if we are going
+        // to enable the grpc-gcp library for other apis.
+        $conf = new ApiConfig();
+        $conf->mergeFromJsonString($string = file_get_contents($conf_path));


Spanner/src/Connection/Grpc.php

@@ -120,11 +122,25 @@ class Grpc implements ConnectionInterface
     */
    private $longRunningGrpcClients;

+    private function enableConnectionManagement($conf_path)


Spanner/src/Connection/Grpc.php

    /**
     * @param array $config [optional]
     */
    public function __construct(array $config = [])
    {
+        if (getenv("ENABLE_GCP_OPTIMIZER") === false) {


Spanner/src/Connection/Grpc.php

+        // to enable the grpc-gcp library for other apis.
+        $conf = new ApiConfig();
+        $conf->mergeFromJsonString($string = file_get_contents($conf_path));
+        $config = new Config($conf);


WeiranFang · 2018-08-13T17:29:56Z

Hi @dwsupplee , to answer you the concern on GAX: there's been a long discussion between us and the gcp client library team (specifically spanner team), and we came into a conclusion that it's better that this extension library is used directly by them, instead of the GAX layer, and it's also done this way in other languages like Python and C#. This extension is highly coupled with the grpc library. Currently we make this as an add-on to fix some GCP specific issues, in future some of the extension logic may even be moved down to the grpc core layer. Also, the gcp extension features are not meant to work on all cloud APIs, currently this channel management work only applies to storage APIs like spanner and bigtable. So we believe the better place to put this library would be the client library directly.

dwsupplee · 2018-08-13T19:05:03Z

@WeiranFang thanks for the explanation and context :).

Moving the logic down to the gRPC core layer sounds great. I didn't mean to suggest that the bulk of that logic should live in GAX, but rather the orchestration of enabling the optimizer could.

For example, we could consider adding the configuration file to the resources directory for the GAPIC. From there, when a user instantiates a GAPIC (or we do, as part of wrapping it for the handwritten layer), GAX would check to see if the configuration file exists and then enable optimization. That would give us fine-grained control over which services use the optimization (it could be controlled by whether or not the configuration file exists) and would make sure we help out our users who directly use the GAPICs just as easily as those using the handwritten veneer layer.

With that said, if you see the configuration file being no longer needed as part of moving the logic into gRPC core - then I am definitely happy with moving forward with what we have now.

WeiranFang · 2018-08-15T17:14:35Z

Hi @dwsupplee , per discussion we had before, I have submitted PR in googleapis/gax-php#207 and googleapis/gapic-generator#2231 . Please take a look whenever you get chance. Thank you!

dwsupplee · 2018-08-16T17:08:26Z

Now that we've ported this over, I think all we need from this PR is to introduce the configuration file, update the GAPIC, and make sure we are updated to the latest version of GAX (when it is released). 👍

Nice work on this @WeiranFang and @ZhouyihaiDing.

dwsupplee · 2018-08-23T14:03:06Z

@WeiranFang / @ZhouyihaiDing since we moved the remaining work for this into another PR, I'll close this out. Please re-open if needed.

ZhouyihaiDing requested a review from jdpedrie as a code owner June 29, 2018 20:17

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2018

ZhouyihaiDing force-pushed the spanner_gcp branch from 4a91341 to 1bb8dd4 Compare June 29, 2018 21:01

ZhouyihaiDing changed the title ~~WIP: Enable connection management~~ WIP: Enable channel management Jun 29, 2018

ZhouyihaiDing mentioned this pull request Jun 29, 2018

GCP library for channel management GoogleCloudPlatform/grpc-gcp-php#12

Merged

ZhouyihaiDing force-pushed the spanner_gcp branch from c55baa3 to bd18c02 Compare June 29, 2018 23:27

tmatsuo reviewed Jul 12, 2018

View reviewed changes

Spanner/src/Connection/Grpc.php Outdated

@@ -152,6 +160,15 @@ public function __construct(array $config = [])

? $config['authHttpHandler']

: null

);

if (isset($config['enable-gcp'])) {

This comment was marked as spam.

Sign in to view

ZhouyihaiDing force-pushed the spanner_gcp branch 7 times, most recently from 603abb9 to 9b0f8c1 Compare July 16, 2018 19:52

ZhouyihaiDing mentioned this pull request Jul 16, 2018

Update composer.json for the first release GoogleCloudPlatform/grpc-gcp-php#15

Merged

WeiranFang reviewed Jul 16, 2018

View reviewed changes

ZhouyihaiDing force-pushed the spanner_gcp branch 4 times, most recently from ceb3cc8 to 18b8a5d Compare July 20, 2018 01:06

ZhouyihaiDing changed the title ~~WIP: Enable channel management~~ Enable gRPC GCP channel management library for the spanner API Jul 20, 2018

Enable gRPC GCP library for channel management

7ff774b

ZhouyihaiDing force-pushed the spanner_gcp branch from 1953193 to 7ff774b Compare August 2, 2018 23:30

jdpedrie approved these changes Aug 6, 2018

View reviewed changes

ZhouyihaiDing force-pushed the spanner_gcp branch 2 times, most recently from 4d8cf72 to ccaf3de Compare August 8, 2018 03:36

ZhouyihaiDing force-pushed the spanner_gcp branch from ccaf3de to f1e8406 Compare August 8, 2018 15:54

ZhouyihaiDing force-pushed the spanner_gcp branch 2 times, most recently from 1badf14 to 7b9699a Compare August 10, 2018 22:18

dwsupplee reviewed Aug 13, 2018

View reviewed changes

ZhouyihaiDing force-pushed the spanner_gcp branch 3 times, most recently from 61165bf to 636d701 Compare August 13, 2018 17:15

ZhouyihaiDing force-pushed the spanner_gcp branch from 636d701 to 0ad8735 Compare August 13, 2018 19:45

remove flag which can be set by the user; Use env instead

543c5c7

ZhouyihaiDing force-pushed the spanner_gcp branch from 0ad8735 to 543c5c7 Compare August 13, 2018 21:46

update the stub option

f8416c5

ZhouyihaiDing force-pushed the spanner_gcp branch from 804dbc0 to f8416c5 Compare August 14, 2018 20:44

WeiranFang mentioned this pull request Aug 15, 2018

Add support to enable grpc_call_invoker googleapis/gax-php#207

Merged

dwsupplee mentioned this pull request Aug 16, 2018

Add gcpApiConfigPath to client default options googleapis/gapic-generator#2231

Merged

dwsupplee closed this Aug 23, 2018

Enable gRPC GCP channel management library for the spanner API #1146

Enable gRPC GCP channel management library for the spanner API #1146

Conversation

ZhouyihaiDing commented Jun 29, 2018 • edited Loading

ZhouyihaiDing commented Jun 29, 2018 • edited Loading

ZhouyihaiDing commented Jun 29, 2018

ZhouyihaiDing commented Jun 29, 2018

jdpedrie commented Jul 2, 2018

ZhouyihaiDing commented Jul 2, 2018

dwsupplee commented Jul 2, 2018 • edited Loading

ZhouyihaiDing commented Jul 2, 2018 • edited Loading

dwsupplee commented Jul 2, 2018

This comment was marked as spam.

ZhouyihaiDing commented Jul 16, 2018 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

jdpedrie left a comment

Choose a reason for hiding this comment

dwsupplee commented Aug 7, 2018

ZhouyihaiDing commented Aug 8, 2018 • edited Loading

dwsupplee commented Aug 10, 2018

ZhouyihaiDing commented Aug 10, 2018 • edited Loading

ZhouyihaiDing commented Aug 10, 2018

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.

WeiranFang commented Aug 13, 2018

dwsupplee commented Aug 13, 2018 • edited Loading

WeiranFang commented Aug 15, 2018

dwsupplee commented Aug 16, 2018 • edited Loading

dwsupplee commented Aug 23, 2018

ZhouyihaiDing commented Jun 29, 2018 •

edited

Loading

ZhouyihaiDing commented Jun 29, 2018 •

edited

Loading

dwsupplee commented Jul 2, 2018 •

edited

Loading

ZhouyihaiDing commented Jul 2, 2018 •

edited

Loading

ZhouyihaiDing commented Jul 16, 2018 •

edited

Loading

ZhouyihaiDing commented Aug 8, 2018 •

edited

Loading

ZhouyihaiDing commented Aug 10, 2018 •

edited

Loading

dwsupplee commented Aug 13, 2018 •

edited

Loading

dwsupplee commented Aug 16, 2018 •

edited

Loading