connecting to Bigtable emulator using environment variable #6067

rahulKQL · 2019-08-12T15:05:30Z

User can now connect to Bigtable emulator in three ways:

By settings hostName:portNum in BIGTABLE_EMULATOR_HOST environment variable.
By providing only the port number within the local workstation.
By providing hostName & port Number.

Now user can connect to Bigtable emulator in three ways: - By settings hostName:portNum in BIGTABLE_EMULATOR_HOST environment variable. - By providing only port number within the local workstation. - By providing hostName & port Number.

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java

igorbernstein2

A couple more things:

please add a log statement notifying the user that the client is targeting the emulator.
in the test rule please add a precondition preventing tests from running when the environment variable is set. The test environment should be controlled via maven profiles not environment variables and the mixture will just create confusion
Please add a safe guard to the instance admin client that prevents its use when the env var is set (the emulator doesn't support instance admin api)

Thanks!

...ud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminSettings.java

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java

...ud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminSettings.java

codecov · 2019-08-19T12:07:17Z

Codecov Report

Merging #6067 into master will increase coverage by 0.26%.
The diff coverage is 20%.

@@             Coverage Diff              @@
##             master    #6067      +/-   ##
============================================
+ Coverage     47.11%   47.38%   +0.26%     
+ Complexity    27195    27182      -13     
============================================
  Files          2522     2523       +1     
  Lines        274264   274599     +335     
  Branches      31326    31383      +57     
============================================
+ Hits         129231   130124     +893     
- Misses       134602   134862     +260     
+ Partials      10431     9613     -818

Impacted Files	Coverage Δ	Complexity Δ
...e/cloud/bigtable/data/v2/BigtableDataSettings.java	`33.33% <16.66%> (-1.45%)`	`4 <2> (+1)`
.../bigtable/admin/v2/BigtableTableAdminSettings.java	`63.63% <18.18%> (-8.11%)`	`8 <2> (+1)`
...gtable/admin/v2/BigtableInstanceAdminSettings.java	`96.55% <50%> (-3.45%)`	`6 <1> (ø)`
...ain/java/com/google/cloud/storage/StorageImpl.java	`77.81% <0%> (-8.39%)`	`98% <0%> (-5%)`
...rc/main/java/com/google/cloud/storage/Storage.java	`79.72% <0%> (-5.45%)`	`0% <0%> (ø)`
...ud/storage/contrib/nio/testing/FakeStorageRpc.java	`59.66% <0%> (-1.29%)`	`44% <0%> (ø)`
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.64% <0%> (-0.18%)`	`1% <0%> (ø)`
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java	`62% <0%> (ø)`	`19% <0%> (ø)`	⬇️
...ogle/cloud/storage/spi/v1/HttpStorageRpcSpans.java	`100% <0%> (ø)`	`3% <0%> (ø)`	⬇️
...rc/main/java/com/google/cloud/storage/HmacKey.java	`5.73% <0%> (ø)`	`0% <0%> (?)`
... and 151 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c9c48c...5b9688b. Read the comment docs.

igorbernstein2

This is looking good.

igorbernstein2 · 2019-08-19T15:56:17Z

...ud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminSettings.java

+      try {
+        port = Integer.parseInt(hostAndPort.substring(hostAndPort.lastIndexOf(":") + 1));
+        return newBuilderForEmulator(hostAndPort.substring(0, hostAndPort.lastIndexOf(":")), port);
+      } catch (NumberFormatException ex) {


Please handle IndexOutOfBoundsException as well

...ud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminSettings.java

igorbernstein2 · 2019-08-19T16:02:07Z

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java

    builder
        .stubSettings()
        .setProjectId("fake-project")
        .setInstanceId("fake-instance")


This diverges from the other clients. I don't think we should set project & instance ids any more

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java

igorbernstein2 · 2019-08-19T16:04:05Z

...ud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminSettings.java

+   * Create a new builder preconfigured to connect to the Bigtable emulator with host & port number.
+   */
+  public static Builder newBuilderForEmulator(String hostname, int port) {
+    Builder builder = new Builder().setProjectId("fake-project").setInstanceId("fake-instance");


I don't think we should be setting the project instance ids to stay consistent with other languages

For Java, we have projectId & instanceId required. Shall I remove the Preconditions checks for projectId & InstanceId?
I would vote to keep these checks and continue with "fake-project" & "fake-instance".

I'm not sure what you mean by that. I'm picturing something along the lines of:

BigtableTableAdminSettings.newBuilderForEmulator("localhost", 1234).setProjectId("blah").setInstanceId("blah2").build()

I think the other client languages require something similar

Thanks for the explanation, I mistook it as emulator without any projectId & instanceId. Have updated with the suggestion. please have a look.

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java

...oogle-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/it/env/TestEnvRule.java

- Updated null check with String.isNullOrEmpty - Handled IndexOutOfBoundsException - fixed java doc in newBuilder() - Adapted with TruthJunit.assume() instead of Assert.assume().

...oogle-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/it/env/TestEnvRule.java

igorbernstein2

LGTM! Thanks for implementing this!

rahulKQL requested a review from igorbernstein2 August 12, 2019 15:05

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 12, 2019

rahulKQL commented Aug 12, 2019

View reviewed changes

...gle-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java Outdated Show resolved Hide resolved

igorbernstein2 reviewed Aug 14, 2019

View reviewed changes

Address feedback comment for emulator configs

db553d3

rahulKQL requested a review from igorbernstein2 August 19, 2019 12:07

igorbernstein2 reviewed Aug 19, 2019

View reviewed changes

Address feedback commets

4ddac32

- Updated null check with String.isNullOrEmpty - Handled IndexOutOfBoundsException - fixed java doc in newBuilder() - Adapted with TruthJunit.assume() instead of Assert.assume().

igorbernstein2 reviewed Aug 19, 2019

View reviewed changes

...oogle-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/it/env/TestEnvRule.java Outdated Show resolved Hide resolved

igorbernstein2 reviewed Aug 19, 2019

View reviewed changes

...oogle-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/it/env/TestEnvRule.java Outdated Show resolved Hide resolved

Addressing more comments

5b9688b

igorbernstein2 approved these changes Aug 19, 2019

View reviewed changes

igorbernstein2 merged commit 13fc06a into googleapis:master Aug 19, 2019

This was referenced Aug 22, 2019

Release google-cloud-java v0.106.0 #6127

Closed

Release google-cloud-java v0.106.0 #6140

Merged

connecting to Bigtable emulator using environment variable #6067

connecting to Bigtable emulator using environment variable #6067

Uh oh!

Conversation

rahulKQL commented Aug 12, 2019

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igorbernstein2 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igorbernstein2 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQL Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQL Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 19, 2019 •

edited

Loading