connecting to Bigtable emulator using environment variable by rahulKQL · Pull Request #6067 · googleapis/google-cloud-java

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.

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!

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

+ 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

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

 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

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

+ * 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.

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

igorbernstein2

LGTM! Thanks for implementing this!

…s#6067) * connecting to Bigtable emulator using environment variable 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. * Address feedback comment for emulator configs * Address feedback commets - Updated null check with String.isNullOrEmpty - Handled IndexOutOfBoundsException - fixed java doc in newBuilder() - Adapted with TruthJunit.assume() instead of Assert.assume(). * Addressing more comments

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

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

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

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

igorbernstein2 reviewed Aug 19, 2019
View reviewed changes

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

Addressing more comments
5b9688b

igorbernstein2 approved these changes Aug 19, 2019
View reviewed changes

igorbernstein2 merged commit 13fc06a into googleapis:masterAug 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

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!

codecovBot 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!

igorbernstein2Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igorbernstein2Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igorbernstein2Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQLAug 19, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQLAug 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

codecovBot commented Aug 19, 2019•
edited
Loading