Added grpc extension. #62

tmatsuo · 2016-03-01T23:47:27Z

No description provided.

JustinBeckwith · 2016-03-01T23:54:44Z

Well that didn't take long! LGTM

tmatsuo · 2016-03-01T23:56:05Z

Yeah, I actually haven't build the docker image locally, so it is possible to fail building. Let's see the travis result.

tmatsuo · 2016-03-02T01:14:44Z

Seems like we still need -beta.

bshaffer · 2016-03-02T01:45:28Z

hey! nice. Is the pecl command sufficient to install all gRPC's dependencies?

bshaffer · 2016-03-02T01:46:00Z

i.e. maybe we should include a gRPC call as part of the tests. To the URL shortener, for example.

tmatsuo · 2016-03-02T19:14:21Z

Sure, it is nicer to have such tests. I'll try to add e2e tests accessing Cloud Pub/Sub (I don't think URL shortner has enabled gRPC).

tmatsuo · 2016-03-03T21:11:30Z

I added an end to end test. Let's see the travis build result.

bshaffer · 2016-03-03T21:24:39Z

testapps/php56_e2e/composer.json

+    "repositories": [
+        {
+            "type": "vcs",
+            "url": "https://github.com/stanley-cheung/Protobuf-PHP"


Is @stanley-cheung interested in submitting his changes datto/protobuf-php? Why are we using his fork?

We tried, but the upstream repo has not responded in a year: drslump/Protobuf-PHP#48

But because he has submited his package to Packagist, we can't submit our fork unless we change the repo name.

That was why I have to resort to this composer.json syntax to use his official package on Packagist in the require section but substitute the actual content with our fork.

Shouldn't this get pulled in implicitly as part of the composer requirements for grpc?

~~That is true. By pulling grpc/grpc, you are also implicitly pulling datto/protobuf-php (with a substituted fork) yes.~~

Edit: sorry, I take that back. The composer.json file you should be looking at should be at the top level of the grpc repo: https://github.com/grpc/grpc/blob/master/composer.json. Packagist only look at composer.json at the top level of a repo. There we did not pull in datto/protobuf-php. I believe at that time we thought pulling in datto/protobuf-php should be left to user of grpc.

Ok, so there's no need for specifying datto/protofuf-php here. I will remove it

No problem, I added it back.

OK gotcha. I advocate making it the default, since it will be very difficult for users to discover the need for them to override with your fork on their own.

So I just tried this again yesterday. Even if the root level grpc/composer.json is going to pull in datto/protobuf-php and with my stanley-cheung override, as a user of grpc/grpc, you are not going to get the stanley-cheung override unless the user also puts that in their own composer.json. In other words, as a user of grpc/grpc, at the minimum, they still need to require grpc/grpc and add the stanley-cheung/protobuf-php override. But then at least they don't need to include datto/protobuf-php. It's not ideal but I can spell it out more in grpc's documentation.

@stanley-cheung Yes, we should add this to composer. Also, are you sure the override is required in the userland composer.json? If so, that seems like a bug in composer. And also if so, we could submit your fork as a new composer package, which would definitely fix the issue.

Requiring the users to override in composer.json rather than just call composer require grpc/grpc is not acceptable DX IMHO. We could fork to GoogleCloudPlatform and submit a new package from there.

Yes I have tried this again and an end-user needs to do this override, and I agree that it's non-ideal. OTOH, this was supposed to be a temporary bridgegap before we have official PHP support in google/protobuf - it's being implemented now. So if we can wait for a few months, this will all go away and we will have the official solution.

tmatsuo · 2016-03-03T21:29:17Z

Stopped the travis build for merging other PR quicker

tmatsuo · 2016-03-04T00:36:08Z

Oooh, of course the auto generated PHP files don't pass our coding style check...

tmatsuo · 2016-03-04T02:18:36Z

testapps/php56_e2e/source-contexts.json

@@ -0,0 +1 @@
+[{"labels": {"category": "remote_repo"}, "context": {"git": {"url": "[email protected]:GoogleCloudPlatform/php-docker.git", "revisionId": "9abd7f7f4adeaf400c774ed667553eec74a385c4"}}}, {"labels": {"category": "remote_repo"}, "context": {"git": {"url": "[email protected]:tmatsuo/php-docker.git", "revisionId": "9abd7f7f4adeaf400c774ed667553eec74a385c4"}}}]


Hey who are you man!

tmatsuo · 2016-03-04T02:46:03Z

I fixed several things, but the test still fails:

2016/03/04 02:29:40 [error] 321#0: *11 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Call to a member function getTopics() on null in /app/web/grpc_pubsub.php on line 36" while reading response header from upstream, client: 172.17.0.5, server: , request: "GET /grpc_pubsub.php HTTP/1.1", upstream: "fastcgi://unix:/var/run/php5-fpm.sock:", host: "tmp-dot-php-mvm-a.appspot.com"

~~Seems like ApplicationDefaultCredentials::getCredentials is not working. I'll investigate tomorrow.~~

tmatsuo · 2016-03-04T03:05:29Z

I think I found a cause!!!

Invalid resource name given (name=projects/)

tmatsuo · 2016-03-04T20:38:23Z

@bshaffer The test is green now. PTAL

bshaffer · 2016-03-04T20:57:56Z

testapps/php56_e2e/web/grpc_pubsub.php

+$call = $client->ListTopics(
+    $req,
+    [],
+    ['call_credentials_callback' => 'updateAuthMetadataCallback']);


It would be nice if we could use the credentials' getUpdateMetadataFunc function for this:

$auth_credentials = ApplicationDefaultCredentials::getCredentials( ['https://www.googleapis.com/auth/pubsub'] ); $call = $client->ListTopics( $req, [], ['call_credentials_callback' => $auth_credentials->getUpdateMetadataFunc())];

But it looks like since the first argument doesn't match up, this won't work. Maybe we should change the google/auth library to handle this? I assume that is what this function is meant for.

hm, so this callback 'updateAuthMetadataCallback' is meant to be seen in gRPC's perspective. It's as if the gRPC core library is saying, "I am ready for the auth credentials. Here's what we know about this call so far, contained in the $context variable (the most important one being $context->service_url, coz after load balancing, user may not know which is the host until now)". So with this $context variable, we ask the auth library for the auth header by calling $auth_credentials->updateMetadata passing $context->service_url to it.

To me these 2 callbacks don't need to lineup because the outer gRPC callback can do more than just Auth. I think it;s OK to leave this as is.

Ok, thanks for the explanation. Is it a problem that the google/auth library isn't doing anything with the service_url?

Also, we could make this cleaner like this:

$auth_credentials = ApplicationDefaultCredentials::getCredentials( ['https://www.googleapis.com/auth/pubsub'] ); $call = $client->ListTopics( $req, [], ['call_credentials_callback' => function($context) use ($auth_credentials) { return $auth_credentials->updateMetadata([], $context->service_url); })];

I believe the $service_url is used only in some xxxCredentials class, like ServiceAccountJwtAccessCredentials but not the others.

I like the inline function change.

ahh yes, I see that now. Very cool. SGTM, thanks @stanley-cheung!

bshaffer · 2016-03-04T20:58:29Z

LGTM

tmatsuo · 2016-03-04T21:47:55Z

I just stopped the build, I will re-run later today.

tmatsuo · 2016-03-09T23:32:51Z

Alright, merging.

Added grpc extension.

tmatsuo assigned bshaffer Mar 1, 2016

Added grpc extension.

efeb7c6

tmatsuo force-pushed the grpc branch from a777f73 to efeb7c6 Compare March 2, 2016 01:13

bshaffer reviewed Mar 3, 2016
View reviewed changes

tmatsuo force-pushed the grpc branch 2 times, most recently from ca6deee to 07c16b8 Compare March 3, 2016 22:21

tmatsuo force-pushed the grpc branch 2 times, most recently from 9abd7f7 to 7f25466 Compare March 4, 2016 02:09

tmatsuo reviewed Mar 4, 2016
View reviewed changes

tmatsuo force-pushed the grpc branch from 7f25466 to efa7675 Compare March 4, 2016 02:34

tmatsuo force-pushed the grpc branch from efa7675 to c69b31a Compare March 4, 2016 03:11

bshaffer reviewed Mar 4, 2016
View reviewed changes

Added an end to end test for grpc extension.

74aed41

tmatsuo force-pushed the grpc branch from c69b31a to 74aed41 Compare March 4, 2016 21:23

tmatsuo pushed a commit that referenced this pull request Mar 9, 2016

Merge pull request #62 from GoogleCloudPlatform/grpc

00570ba

Added grpc extension.

tmatsuo merged commit 00570ba into master Mar 9, 2016

tmatsuo deleted the grpc branch March 9, 2016 23:33

tmatsuo mentioned this pull request Apr 16, 2016

What's the plan for supporting the GAE services over RPC? #54

Closed

		@@ -0,0 +1 @@
		[{"labels": {"category": "remote_repo"}, "context": {"git": {"url": "[email protected]:GoogleCloudPlatform/php-docker.git", "revisionId": "9abd7f7f4adeaf400c774ed667553eec74a385c4"}}}, {"labels": {"category": "remote_repo"}, "context": {"git": {"url": "[email protected]:tmatsuo/php-docker.git", "revisionId": "9abd7f7f4adeaf400c774ed667553eec74a385c4"}}}] No newline at end of file

Added grpc extension. #62

Added grpc extension. #62

Uh oh!

Conversation

tmatsuo commented Mar 1, 2016

Uh oh!

JustinBeckwith commented Mar 1, 2016

Uh oh!

tmatsuo commented Mar 1, 2016

Uh oh!

tmatsuo commented Mar 2, 2016

Uh oh!

bshaffer commented Mar 2, 2016

Uh oh!

bshaffer commented Mar 2, 2016

Uh oh!

tmatsuo commented Mar 2, 2016

Uh oh!

tmatsuo commented Mar 3, 2016

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!

tmatsuo commented Mar 3, 2016

Uh oh!

tmatsuo commented Mar 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo commented Mar 4, 2016

Uh oh!

tmatsuo commented Mar 4, 2016

Uh oh!

tmatsuo commented Mar 4, 2016

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!

bshaffer commented Mar 4, 2016

Uh oh!

tmatsuo commented Mar 4, 2016

Uh oh!

tmatsuo commented Mar 9, 2016

Uh oh!

Uh oh!