GCP library for channel management by ZhouyihaiDing · Pull Request #12 · GoogleCloudPlatform/grpc-gcp-php

ZhouyihaiDing · 2018-06-25T16:29:21Z

Code sample:

        $conf = new \Grpc\Gcp\ApiConfig();
        $conf->mergeFromJsonString($string);

        $config = new \Grpc\GCP\Config($hostname, $conf);
        $credentials = \Grpc\ChannelCredentials::createSsl();
        $auth = ApplicationDefaultCredentials::getCredentials();
        $opts = [
            'credentials' => $credentials,
            'update_metadata' => $auth->getUpdateMetadataFunc(),
            'grpc_call_invoker' => $config->callInvoker(),
        ];

        $this->stub = new SpannerGrpcClient($hostname, $opts);

Reference: implementation in python

WeiranFang · 2018-06-27T17:33:28Z

src/lib/GCPCallInvoker.php

+        return $this->real_call;
+    }
+
+    // Public funtions are rewriting all methods inside UnaryCall


This GCPUnaryCall seems only rewriting partial public methods. The original Grpc\UnaryCall extends Grpc\AbstractCall (https://github.com/grpc/grpc/blob/master/src/php/lib/Grpc/AbstractCall.php), which implements some common methods including getMetadata, getTrailingMetadata, getPeer, cancel, setCallCredentials. These methods are shared by all the 4 subclass type calls.

If we want to rewrite all the public functions of the original calls, should we also need to overwrite these common functions? We can either implement them in the superclass GcpBaseCall, or respectively in the four subclasses. Another option is to make the GcpBaseCall inherit from Grpc\AbstractCall.

You are right. I will overwrite those functions inside GCPBaseCall. Thanks!

TeBoring · 2018-06-28T20:43:50Z

LGTM for php style only

chingor13

A few notes on style. The cloud libraries have used the PSR-2 coding style guide.

Each class should be in its own separate file and it should allow the composer autoloader to require the files. If you do this, you won't need the extra requires in the test files.
Variables should be camel-cased
Private classes should not be prefixed with _. You should annotate them in the class descriptions.
Private variables should be camelcased and not be prefixed with _. Access modifiers is enough.
src/lib/ => src/

chingor13 · 2018-06-28T21:13:57Z

src/lib/GCPConfig.php

+ */
+namespace Grpc\GCP;
+
+use Psr\Cache\CacheItemPoolInterface;


Need to add psr/cache to the dependencies in composer.json

chingor13 · 2018-06-28T21:18:26Z

src/lib/GCPCallInvoker.php

+        $this->affinity_key = null;
+        if($this->_affinity) {
+            $command = $this->_affinity['command'];
+            if ($command == 'BOUND' || $command == 'UNBIND') {


These should probably be constants. A few other spots below too.

Done. Thanks!
I create new files for each class defined and use php-cs-fixer to help me update the code.

I added more comments; removed methods start with '_', and also removed unnecessary comments/codes. Thank you!

bshaffer · 2018-06-28T23:22:57Z

composer.json

@@ -0,0 +1,25 @@
+{
+  "name": "grpc/grpc-gcp",


alternatively this could be google/cloud-grpc

This would only make sense if we saw this as possibly going into the google-cloud-php repository eventually, however.

Done. Thanks!

Hi Brent,
We think it it better to be named with gcp like grpc-gcp, because it is designed to make gcp better and solve the gcp specific issues. OSS users probably won't use this library.

bshaffer · 2018-06-28T23:23:32Z

src/lib/ChannelRef.php

+ */
+namespace Grpc\GCP;
+
+class _ChannelRef


starting classes with underscores is not idiomatic to PHP. use final instead?

bshaffer · 2018-06-28T23:24:00Z

src/lib/ChannelRef.php

+ * limitations under the License.
+ *
+ */
+namespace Grpc\GCP;


Namespace would be better as Google\Cloud\Grpc

bshaffer · 2018-06-28T23:24:23Z

src/lib/ChannelRef.php

+// If not, we can use $real_channel directly instead of creating a new one.
+// It is useful to handle 'force_new' channel option.
+// TODO(ddyihai): remove it once the serialzer handler for \Grpc\Channel is implemented.
+class _HasDeserialized implements \Serializable {


Classes should each get their own project file and not share a file with other classes. This is to follow PSR-0 autoloading standards.

Done. I create a new file for each class defined now. Thanks!

chingor13 · 2018-06-29T16:24:39Z

tests/grpc_unit_test/data/ca.pem

@@ -0,0 +1,15 @@
+-----BEGIN CERTIFICATE-----


Just making sure these aren't real credentials.

It is only used for testing. I copied it from the grpc/grpc's php unit tests. I didn't find out how I can specify certain files uploaded to the packagist because only files under src/ are needed.

chingor13 · 2018-06-29T16:25:20Z

src/GCPCallInvoker.php

+    }
+}
+
+abstract class GcpBaseCall


These other classes should move to separate files.

Done. Sorry about that.

I will add more comments for those classes and methods.

bshaffer · 2018-06-29T17:15:35Z

composer.json

+      "": ["src/generated/"]
+    }
+  }
+}


This is super picky but most of our composer.json files use 4 spaces instead of two

bshaffer · 2018-06-29T17:16:00Z

src/ChannelRef.php

+class ChannelRef
+{
+    // $opts has all information except Credentials for creating a Grpc\Channel
+    // except Grpc\Credentials.


nit but this line can be removed!

bshaffer · 2018-06-29T17:17:15Z

tests/fpm/php-fpm1.php

+putenv("GOOGLE_APPLICATION_CREDENTIALS=./../grpc-gcp.json");
+
+require_once(dirname(__FILE__) . '/../vendor/autoload.php');
+require_once(dirname(__FILE__) . '/../../vendor/autoload.php');


Instead of dirname(__FILE__) you can use simply __DIR__

Done. Thank you!
I am trying to run the spanner tests under google-cloud-php repo so I can add more tests later.
However, I cannot get any test passed when I am following exactly what travis does. I noticed that all files under keys/ are empty. Do you know are there extra steps I should do to run the tests?
Thanks!

ZhouyihaiDing · 2018-06-29T21:26:47Z

Hi all,
I create a simple PR to google-cloud-php, which describes my idea about how to enable the channel management support in the cloud API, which looks like:

$client = new SpannerClient(['enable_gcp' => true]);

It seems SpannerClient and SpannerGapicClient are both generated according to the comment, thus I need some help to find where the generator is to insert the gcp support.
Please take a look and provide suggestions, thank you!

change SESSION to apcu; Add CacheItemPoolInterface add composer.json update code style create file BaseCall/UnaryCall/StreamCall update composer.json, __DIR__ and nit fix typo, add more comments, remove unnecessary things

WeiranFang · 2018-07-09T17:43:07Z

This repo needs to be published. I'll merge this PR and clean some commit history before it gets public. However the package itself is not ready to be released yet. More fixes can be done in future PRs before release.

ZhouyihaiDing force-pushed the gcp_extension branch from 8c67e04 to ffebd0b Compare June 25, 2018 16:35

ZhouyihaiDing mentioned this pull request Jun 25, 2018

PHP: add call invoker grpc/grpc#15749

Merged

ZhouyihaiDing force-pushed the gcp_extension branch 6 times, most recently from 6ee5f06 to ee7c813 Compare June 26, 2018 23:41

WeiranFang reviewed Jun 27, 2018

View reviewed changes

ZhouyihaiDing force-pushed the gcp_extension branch 4 times, most recently from 5a9117f to a2c29ff Compare June 28, 2018 16:24

TeBoring approved these changes Jun 28, 2018

View reviewed changes

chingor13 reviewed Jun 28, 2018

View reviewed changes

ZhouyihaiDing force-pushed the gcp_extension branch from ba6de02 to 612fe82 Compare June 28, 2018 21:24

bshaffer reviewed Jun 28, 2018

View reviewed changes

ZhouyihaiDing force-pushed the gcp_extension branch 2 times, most recently from 1ef8b2d to 2f2a95e Compare June 29, 2018 15:40

chingor13 reviewed Jun 29, 2018

View reviewed changes

ZhouyihaiDing force-pushed the gcp_extension branch from ce81280 to 13f9ad9 Compare June 29, 2018 16:44

bshaffer reviewed Jun 29, 2018

View reviewed changes

ZhouyihaiDing force-pushed the gcp_extension branch 2 times, most recently from a2fe458 to 62ad011 Compare June 29, 2018 18:05

ZhouyihaiDing force-pushed the gcp_extension branch from 62ad011 to b397740 Compare June 29, 2018 18:11

ZhouyihaiDing changed the title ~~GCP extension via call invoker~~ GCP library for connection management Jun 29, 2018

ZhouyihaiDing mentioned this pull request Jun 29, 2018

Enable gRPC GCP channel management library for the spanner API googleapis/google-cloud-php#1146

Closed

ZhouyihaiDing force-pushed the gcp_extension branch 3 times, most recently from c9b20d1 to f4a5631 Compare June 29, 2018 20:52

ZhouyihaiDing changed the title ~~GCP library for connection management~~ GCP library for channel management Jun 29, 2018

ZhouyihaiDing force-pushed the gcp_extension branch 3 times, most recently from dfcbec3 to 40fb667 Compare June 29, 2018 22:38

ZhouyihaiDing force-pushed the master branch from 8ce5ce4 to 40f1d41 Compare July 2, 2018 20:36

ZhouyihaiDing force-pushed the gcp_extension branch 2 times, most recently from 8eca422 to 743618a Compare July 2, 2018 20:41

GCP extension via call invoker

63596ef

change SESSION to apcu; Add CacheItemPoolInterface add composer.json update code style create file BaseCall/UnaryCall/StreamCall update composer.json, __DIR__ and nit fix typo, add more comments, remove unnecessary things

ZhouyihaiDing force-pushed the gcp_extension branch from 743618a to 63596ef Compare July 6, 2018 17:20

WeiranFang merged commit 4903679 into GoogleCloudPlatform:master Jul 9, 2018

Conversation

ZhouyihaiDing commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TeBoring commented Jun 28, 2018

Uh oh!

chingor13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhouyihaiDing Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhouyihaiDing commented Jun 29, 2018

Uh oh!

WeiranFang commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ZhouyihaiDing commented Jun 25, 2018 •

edited

Loading

ZhouyihaiDing Jun 29, 2018 •

edited

Loading