-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GCP library for channel management #12
GCP library for channel management #12
Conversation
8c67e04
to
ffebd0b
Compare
6ee5f06
to
ee7c813
Compare
src/lib/GCPCallInvoker.php
Outdated
return $this->real_call; | ||
} | ||
|
||
// Public funtions are rewriting all methods inside UnaryCall |
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 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.
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.
You are right. I will overwrite those functions inside GCPBaseCall. Thanks!
5a9117f
to
a2c29ff
Compare
LGTM for php style only |
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.
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/
src/lib/GCPConfig.php
Outdated
*/ | ||
namespace Grpc\GCP; | ||
|
||
use Psr\Cache\CacheItemPoolInterface; |
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.
Need to add psr/cache
to the dependencies in composer.json
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.
Done.
src/lib/GCPCallInvoker.php
Outdated
$this->affinity_key = null; | ||
if($this->_affinity) { | ||
$command = $this->_affinity['command']; | ||
if ($command == 'BOUND' || $command == 'UNBIND') { |
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.
These should probably be constants. A few other spots below too.
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.
Done. Thanks!
I create new files for each class defined and use php-cs-fixer to help me update the code.
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 added more comments; removed methods start with '_', and also removed unnecessary comments/codes. Thank you!
ba6de02
to
612fe82
Compare
composer.json
Outdated
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "grpc/grpc-gcp", |
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.
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.
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.
Done. Thanks!
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.
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.
src/lib/ChannelRef.php
Outdated
*/ | ||
namespace Grpc\GCP; | ||
|
||
class _ChannelRef |
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.
starting classes with underscores is not idiomatic to PHP. use final
instead?
src/lib/ChannelRef.php
Outdated
* limitations under the License. | ||
* | ||
*/ | ||
namespace Grpc\GCP; |
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.
Namespace would be better as Google\Cloud\Grpc
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.
Done.
src/lib/ChannelRef.php
Outdated
// 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 { |
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.
Classes should each get their own project file and not share a file with other classes. This is to follow PSR-0 autoloading standards.
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.
Done. I create a new file for each class defined now. Thanks!
1ef8b2d
to
2f2a95e
Compare
@@ -0,0 +1,15 @@ | |||
-----BEGIN CERTIFICATE----- |
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.
Just making sure these aren't real credentials.
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.
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.
src/GCPCallInvoker.php
Outdated
} | ||
} | ||
|
||
abstract class GcpBaseCall |
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.
These other classes should move to separate files.
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.
Done. Sorry about that.
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 will add more comments for those classes and methods.
ce81280
to
13f9ad9
Compare
composer.json
Outdated
"": ["src/generated/"] | ||
} | ||
} | ||
} |
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 super picky but most of our composer.json
files use 4 spaces instead of two
src/ChannelRef.php
Outdated
class ChannelRef | ||
{ | ||
// $opts has all information except Credentials for creating a Grpc\Channel | ||
// except Grpc\Credentials. |
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.
nit but this line can be removed!
tests/fpm/php-fpm1.php
Outdated
putenv("GOOGLE_APPLICATION_CREDENTIALS=./../grpc-gcp.json"); | ||
|
||
require_once(dirname(__FILE__) . '/../vendor/autoload.php'); | ||
require_once(dirname(__FILE__) . '/../../vendor/autoload.php'); |
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.
Instead of dirname(__FILE__)
you can use simply __DIR__
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.
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!
a2fe458
to
62ad011
Compare
62ad011
to
b397740
Compare
c9b20d1
to
f4a5631
Compare
Hi all,
It seems |
dfcbec3
to
40fb667
Compare
8eca422
to
743618a
Compare
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
743618a
to
63596ef
Compare
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. |
Code sample:
Reference: implementation in python