Bigtable createInstance Public Api by mohitq · Pull Request #1086 · googleapis/google-cloud-php

mohitq · 2018-06-07T12:33:46Z

Review Public API

sduskis · 2018-06-08T15:32:53Z

@mohitq The tests are failing. They need to pass before a review can happen.

Sign in to view

+use Google\Auth\FetchAuthTokenInterface;
+use Google\Cloud\Core\ClientTrait;
+use Google\Cloud\Core\Exception\GoogleException;
+use Google\Cloud\Core\Int64;


Sign in to view

+use Google\Cloud\Core\LongRunning\LROTrait;
+use Google\Cloud\Bigtable\Admin\V2\BigtableInstanceAdminClient as InstanceAdminClient;
+use Google\Cloud\Bigtable\Connection\Grpc;
+use Google\Cloud\Bigtable\Connection\LongRunningConnection;


sduskis · 2018-06-12T13:47:55Z

@jdpedrie, do you have any idea why the builds failed?

jdpedrie · 2018-06-12T14:10:36Z

@sduskis appveyor can be a bit flaky when installing dependencies. I've restarted the build there. There's no action that is required to fix that so long as the restarted build successfully gets through the install step.

On travis, there was a segfault, which can sometimes be fixed with a re-run, but more often indicates an underlying problem which can be somewhat difficult to debug. I'm re-running that job. If it fails again I'll see if I can uncover the problem.

Sign in to view

+ * $ pecl install protobuf
+ * ```
+ *
+ * Example


Sign in to view

+ *         @type string $pageToken A previously-returned page token used to
+ *               resume the loading of results from a specific point.
+ *     }
+ *     @return ItemIterator<InstanceConfiguration>


Sign in to view

+use Google\Cloud\Core\LongRunning\LROTrait;
+use Google\Cloud\Bigtable\Admin\V2\BigtableInstanceAdminClient as InstanceAdminClient;
+use Google\Cloud\Bigtable\Connection\Grpc;
+use Google\Cloud\Bigtable\Connection\LongRunningConnection;


dwsupplee · 2018-06-12T14:37:13Z

@mohitq Did you want to include tests in this PR or as a follow up?

mohitq · 2018-06-14T12:25:17Z

@dwsupplee Is it possible this PR without tests? If yes, then i will create a new PR for tests.

Sign in to view

+ * @method resumeOperation() {
+ *     Resume a Long Running Operation
+ *
+ *     Example


Sign in to view

+    /**
+     * @param ConnectionInterface $connection
+     */
+    public function __construct(ConnectionInterface $connection)


dwsupplee · 2018-06-14T15:10:42Z

@mohitq I would recommend to write tests alongside the code, so that if the tests reveal any issues you don't need to circle back again. However, if you would prefer to add them later I don't have a major issue with that.

jdpedrie · 2018-06-18T13:52:09Z

@mohitq looks like you've got some snippet test failures. Don't forget, you can run all the tests except the system tests locally using dev/sh/tests. You can also run test suites individually:

# unit tests
$ vendor/bin/phpunit

# snippet tests
$ vendor/bin/phpunit -c phpunit-snippets.xml.dist

If you want to run just the bigtable tests, use the --group option, like so:

# run bigtable unit tests
$ vendor/bin/phpunit --group=bigtable`

# run bigtable snippet tests
$ vendor/bin/phpunit -c phpunit-snippets.xml.dist --group=bigtable

Apologies if I'm reiterating information you already know. Just want to be sure. :)

sduskis · 2018-06-25T13:52:46Z

@jdpedrie and @dwsupplee I think that we're ready for a review.

Sign in to view

+            $id = $value['clusterId'];
+            $clusters[$id] = [
+                'location' => InstanceAdminClient::locationName($this->projectId, $value['locationId']),
+                'serveNodes' => isset($value['nodeCount']) ? $value['nodeCount'] : 1,


Sign in to view

+     *                 string $clusterId
+     *                 string $locationId
+     *                 int $nodeCount **Defaults to** `1`.
+     *                 int $storageType **Defaults to** `0`.


Sign in to view

+     *     Configuration options
+     *
+     *     @type string $displayName **Defaults to** the value of $name.
+     *     @type int $instanceType **Defaults to** `0`.


Sign in to view

+     *           requests specifically for authentication.
+     *     @type FetchAuthTokenInterface $credentialsFetcher A credentials
+     *           fetcher instance.
+     *     @type callable $httpHandler A handler used to deliver Psr7 requests.


mohitq · 2018-06-25T15:11:57Z

Updated as per comment.

Sign in to view

+     *
+     *     @type string $displayName **Defaults to** the value of $instanceId.
+     *     @type int $instanceType  Possible values include `Instance_Type::PRODUCTION`
+     *                              and `Instance_Type::DEVELOPMENT`.


mohitq · 2018-06-26T13:20:32Z

@jdpedrie Updated all comment changes.

Sign in to view

+     *
+     *     @type string $projectId The project ID from the Google Developer's
+     *           Console.
+     *     @type array $authCacheOptions Cache configuration options.


Sign in to view

+     *     @type string $projectId The project ID from the Google Developer's
+     *           Console.
+     *     @type array $authCacheOptions Cache configuration options.
+     *     @type callable $authHttpHandler A handler used to deliver Psr7


jdpedrie

Client is looking very good. I found a couple of style issues and a couple of suggested improvements, but this is getting close.

The new classes need unit test coverage. I'd prefer if it we could test as we go, rather than trying to play catch up later. If you feel strongly otherwise, please let me know and we'll talk about it.

Sign in to view

+     *           **Defaults to**`Instance_Type::TYPE_UNSPECIFIED`.
+     *     @type array $labels For more information, see
+     *           [Using labels to organize Google Cloud Platform resources](https://cloudplatform.googleblog.com/2015/10/using-labels-to-organize-Google-Cloud-Platform-resources.html).
+     *     @type array $clusters [] {


Sign in to view

+     * }
+     * @return array
+    */
+    private function clustersArray($args)


Sign in to view

+            $clusters[$id] = [
+                'location' => InstanceAdminClient::locationName($this->projectId, $value['locationId']),
+                'serveNodes' => isset($value['serveNodes']) ? $value['serveNodes'] : '',
+                'defaultStorageType' => isset($value['storageType'])?$value['storageType']


dwsupplee

Really nice progress @mohitq, this is shaping up well.

Sign in to view

+     *     @type array $authCacheOptions Cache configuration options.
+     *     @type callable $authHttpHandler A handler used to deliver Psr7
+     *           requests specifically for authentication.
+     *     @type FetchAuthTokenInterface $credentialsFetcher A credentials


Sign in to view

+
+namespace Google\Cloud\Bigtable;
+
+use Google\Cloud\Core\ArrayTrait;


Sign in to view

+     *     @type int $retries Number of retries for a failed request.
+     *           **Defaults to** `3`.
+     * }
+     * @throws GoogleException If the gRPC extension is not enabled.


Sign in to view

+     * ```
+     *
+     * @codingStandardsIgnoreStart
+     * @see https://cloud.google.com/bigtable/docs/reference/admin/rpc/google.bigtable.admin.v2#CreateInstanceRequest CreateInstanceRequest


Sign in to view

+     *
+     *     @type string $displayName **Defaults to** the value of $instanceId.
+     *     @type int $instanceType  Possible values include `Instance_Type::PRODUCTION` and `Instance_Type::DEVELOPMENT`.
+     *           **Defaults to**`Instance_Type::TYPE_UNSPECIFIED`.


Sign in to view

+    public function exists(array $options = [])
+    {
+        try {
+            $this->reload($options = []);


Sign in to view

+     *     Configuration options
+     *
+     *     @type string $displayName **Defaults to** the value of $instanceId.
+     *     @type int $instanceType  Possible values include `Instance_Type::PRODUCTION` and `Instance_Type::DEVELOPMENT`.


Sign in to view

+     */
+    private function fullyQualifiedInstanceName($instanceId, $projectId)
+    {
+        try {


Sign in to view

+use Google\Cloud\Bigtable\Admin\V2\BigtableInstanceAdminClient as InstanceAdminClient;
+use Google\Cloud\Bigtable\BigtableClient;
+use Google\Cloud\Bigtable\Instance;
+use Google\Cloud\Bigtable\Connection\ConnectionInterface;


Sign in to view

+    }
+
+    /**
+     * @group bigtabladmin


sduskis · 2018-07-02T13:59:08Z

This PR got larger than it needed to be, and still has some points of contention. I'd propose that we break up this PR into the following PRs:

Finally, add BigtableClient.

How does this sound?

Sign in to view

+     *         int $storageType The storage media type for persisting Bigtable data. Possible values include `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_SSD` and `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_HDD`.
+     *             **Defaults to** `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_UNSPECIFIED`.
+     *     }]
+     *     Ex. [


Sign in to view

+     * @param array $args
+     */
+    public function getOperation(array $args);
+    /**


Sign in to view

+
+/**
+ * @group bigtable
+ * @group bigtable-admin


Sign in to view

+
+        $clustersArray = [];
+        foreach ($clusters as $value) {
+            $id = $value['clusterId'];


Sign in to view

+    private function cluster($args)
+    {
+        $cluster = [];
+        $cluster['location'] = InstanceAdminClient::locationName($this->projectId, $args['locationId']);


dwsupplee · 2018-07-02T18:26:38Z

Sounds good to me @sduskis

sduskis · 2018-07-03T16:54:37Z

Long running connection PR was submitted: #1148

dwsupplee · 2018-07-03T17:43:56Z

#1148 is merged, if we could please rebase now.

Sign in to view

+ * $instance = $bigtable->instance('my-instance');
+ * ```
+ *
+ * @method resumeOperation() {


Sign in to view

+ *     @param array $info [optional] The operation data.
+ *     @return LongRunningOperation
+ * }
+ * @method longRunningOperations() {


sduskis

@dwsupplee, I rebased.

sduskis · 2018-07-03T18:09:20Z

The GrpcTest was updated in #1149.

dwsupplee · 2018-07-03T18:17:58Z

Thank you @sduskis. I hope you don't mind, I edited your comment to reflect the status of the items on the PR.

It looks like we just need this last round of reviews to be addressed. From there, it would be great if @jdpedrie / @fiboknacky don't mind doing a final pass to make sure everything looks good to them as well.

fiboknacky

I'm quite confused now.
We have #1158 open for similar changes.
Shall we continue reviewing this PR or move to do on that PR instead?

Sign in to view

+     *         int $storageType The storage media type for persisting Bigtable data. Possible values include `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_SSD` and `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_HDD`.
+     *             **Defaults to** `Google\Cloud\Bigtable\Instance::STORAGE_TYPE_UNSPECIFIED`.
+     *     }]
+     *     Ex. [


sduskis · 2018-07-09T13:37:46Z

Closing this in favor of #1158

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

jdpedrie changed the title ~~createIntsance Public Api~~ createInstance Public Api Jun 7, 2018

jdpedrie added api: bigquery Issues related to the BigQuery API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 8, 2018

dwsupplee added api: bigtable Issues related to the Bigtable API. and removed api: bigquery Issues related to the BigQuery API. labels Jun 8, 2018

fiboknacky reviewed Jun 12, 2018

View reviewed changes

dwsupplee reviewed Jun 12, 2018

View reviewed changes

Comment thread Bigtable/src/BigtableClient.php Outdated

* $ pecl install protobuf

* ```

*

* Example

This comment was marked as spam.

Sign in to view

jdpedrie reviewed Jun 12, 2018

View reviewed changes

mohitq changed the title ~~createInstance Public Api~~ Bigtable createInstance Public Api Jun 13, 2018

jdpedrie reviewed Jun 14, 2018

View reviewed changes

Comment thread Bigtable/src/BigtableClient.php Outdated

* @method resumeOperation() {

* Resume a Long Running Operation

*

* Example

This comment was marked as spam.

Sign in to view

fiboknacky reviewed Jun 14, 2018

View reviewed changes

Comment thread Bigtable/src/Connection/LongRunningConnection.php

/**

* @param ConnectionInterface $connection

*/

public function __construct(ConnectionInterface $connection)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

jdpedrie mentioned this pull request Jun 20, 2018

Bigtable Table Functions #1115

Closed

sduskis suggested changes Jun 25, 2018

View reviewed changes

sduskis reviewed Jun 25, 2018

View reviewed changes

jdpedrie reviewed Jun 25, 2018

View reviewed changes

Comment thread Bigtable/src/BigtableClient.php Outdated

*

* @type string $displayName **Defaults to** the value of $instanceId.

* @type int $instanceType Possible values include `Instance_Type::PRODUCTION`

* and `Instance_Type::DEVELOPMENT`.

This comment was marked as spam.

Sign in to view

fiboknacky reviewed Jun 26, 2018

View reviewed changes

jdpedrie suggested changes Jun 26, 2018

View reviewed changes

dwsupplee suggested changes Jun 26, 2018

View reviewed changes

fiboknacky mentioned this pull request Jun 27, 2018

[Bigtable] Added methods for manipulating IAM permissions to Grpc.php #1129

Merged

mohitq added 12 commits July 2, 2018 10:18

Fix snippet tests

8484525

Remove InstanceConfiguration and fix changes.

de2b1d2

CreateInstance, cluster arguments in options

bc83ec3

UPdate PR changes.

15f4038

Fix comment.

8a0b519

Merge Comment.

98f1036

Addressing PR comments.

5443b52

Updated comments.

755a4d8

Unit tests of BigtableClient and Instance.

9cce2c0

Fixing indentation.

ae08783

Fixing indentation.

afef330

Addressing comments.

c1ed412

mohitq force-pushed the bigtable_public-api branch from 0c63349 to c1ed412 Compare July 2, 2018 04:50

Rebased PR updates.

07cdc69

dwsupplee reviewed Jul 2, 2018

View reviewed changes

dwsupplee mentioned this pull request Jul 2, 2018

BIgtable longrunnig connection #1148

Merged

Merge branch 'bigtable' into bigtable_public-api

afa527b

sduskis suggested changes Jul 3, 2018

View reviewed changes

sduskis reviewed Jul 3, 2018

View reviewed changes

sduskis mentioned this pull request Jul 3, 2018

Bigtable Create Instance Public API #1158

Merged

Updating link in Instance.php

5e1585f

fiboknacky reviewed Jul 4, 2018

View reviewed changes

sduskis closed this Jul 9, 2018


		namespace Google\Cloud\Bigtable;

		use Google\Cloud\Core\ArrayTrait;

Conversation

mohitq commented Jun 7, 2018

Uh oh!

sduskis commented Jun 8, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

sduskis commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dwsupplee commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dwsupplee commented Jun 14, 2018

Uh oh!

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

Uh oh!

Uh oh!

sduskis commented Jun 25, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

mohitq commented Jun 25, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

mohitq commented Jun 26, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

jdpedrie left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dwsupplee left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

jdpedrie commented Jun 12, 2018 •

edited

Loading

mohitq commented Jun 14, 2018 •

edited

Loading

jdpedrie commented Jun 18, 2018 •

edited

Loading

sduskis commented Jul 2, 2018 •

edited by dwsupplee

Loading