Skip to content
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

fix: Make refreshing channel compatible with BigtableDataClientFactory #474

Merged
merged 6 commits into from Oct 22, 2020

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Oct 16, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #458 ☕️

@mutianf mutianf requested a review from as a code owner Oct 16, 2020
@google-cla google-cla bot added the cla: yes label Oct 16, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 16, 2020

Codecov Report

Merging #474 into master will increase coverage by 0.49%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #474      +/-   ##
============================================
+ Coverage     80.62%   81.11%   +0.49%     
- Complexity     1116     1124       +8     
============================================
  Files           106      106              
  Lines          6942     6965      +23     
  Branches        369      368       -1     
============================================
+ Hits           5597     5650      +53     
+ Misses         1147     1117      -30     
  Partials        198      198              
Impacted Files Coverage Δ Complexity Δ
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 97.43% <ø> (+0.51%) 21.00 <0.00> (-1.00) ⬆️
...ud/bigtable/data/v2/BigtableDataClientFactory.java 95.65% <100.00%> (+0.09%) 9.00 <0.00> (ø)
...om/google/cloud/bigtable/emulator/v2/Emulator.java 60.65% <0.00%> (+0.81%) 14.00% <0.00%> (ø%)
...e/cloud/bigtable/data/v2/BigtableDataSettings.java 71.42% <0.00%> (+5.71%) 7.00% <0.00%> (ø%)
...ud/bigtable/admin/v2/BigtableTableAdminClient.java 84.58% <0.00%> (+12.41%) 44.00% <0.00%> (+9.00%)

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 a3bb371...d26e17a. Read the comment docs.

Copy link

@yoshi-code-bot yoshi-code-bot left a comment

Some suggestions could not be made:

  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactory.java
    • lines 26-26
  • google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java
    • lines 93-106

@mutianf mutianf changed the title Make refreshing channel compatible with BigtableDataClientFactory fix: Make refreshing channel compatible with BigtableDataClientFactory Oct 16, 2020
@product-auto-label product-auto-label bot added the api: bigtable label Oct 17, 2020
Copy link

@yoshi-code-bot yoshi-code-bot left a comment

Some suggestions could not be made:

  • google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettingsTest.java
    • lines 28-28

@kolea2 kolea2 added the kokoro:force-run label Oct 20, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Oct 20, 2020
try {
channelProviderBuilder.setChannelPrimer(
BigtableChannelPrimer.create(
getCredentialsProvider().getCredentials(),
Copy link
Contributor

@igorbernstein2 igorbernstein2 Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a subtle bug here:

If the original CredentialsProvider was a GoogleCredentialsProvider then each time getCredentials is called, it will be a new instance, which means the credentials used for refreshing will diverge from the normal request credentials.

I think you need to wrap reset the credentials to be FixedCredentials here.

Also please move the getCredentials line into its own try/catch its kinda hard to see which line can throw the IOException

Copy link
Collaborator

@kolea2 kolea2 Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add here, can we add a test to ensure the credentials are consistent?

Copy link
Contributor Author

@mutianf mutianf Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated BigtableDataClientFactoryTest.testCreateWithRefreshingChannel test to verify that getCredentials is only called once by the credentialProvider we set (similar to test testNewClientsShareTransportChannel.testNewClientsShareTransportChannel). Also added a test in EnhancedBigtableStubSettingsTest to make sure it's returning the same credentials.

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

lgtm


@Override
public void transportTerminated(Attributes transportAttrs) {
attributes.add(transportAttrs);
Copy link
Contributor

@igorbernstein2 igorbernstein2 Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to split termination attrs from setup attrs

factory.createForAppProfile("other-appprofile");
factory.createForInstance("other-project", "other-instance");

// Make sure that only 1 instance is created by each provider
Copy link
Contributor

@igorbernstein2 igorbernstein2 Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that only 1 instance is created for all clients

@mutianf mutianf merged commit fc74164 into googleapis:master Oct 22, 2020
19 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 23, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.17.0](https://www.github.com/googleapis/java-bigtable/compare/v1.16.2...v1.17.0) (2020-10-23)


### Features

* attemp DirectPath by default ([#467](https://www.github.com/googleapis/java-bigtable/issues/467)) ([89c622d](https://www.github.com/googleapis/java-bigtable/commit/89c622da6038067892687af3edafae743465eda7))
* backup level IAM ([#450](https://www.github.com/googleapis/java-bigtable/issues/450)) ([f38a8ec](https://www.github.com/googleapis/java-bigtable/commit/f38a8ecdc6164d081ef96f748ea37bd62b29b419))
* Implement toString for Bigtable*Settings ([#488](https://www.github.com/googleapis/java-bigtable/issues/488)) ([4d821f8](https://www.github.com/googleapis/java-bigtable/commit/4d821f85ceb237c8e449243ff8c80fb94e22ad51))


### Bug Fixes

* Make refreshing channel compatible with BigtableDataClientFactory ([#474](https://www.github.com/googleapis/java-bigtable/issues/474)) ([fc74164](https://www.github.com/googleapis/java-bigtable/commit/fc741645536e01fac772136bc8346f73ff95e600))


### Documentation

* fix formatting ([#476](https://www.github.com/googleapis/java-bigtable/issues/476)) ([eb24569](https://www.github.com/googleapis/java-bigtable/commit/eb24569e53f9d2b7fde50748c840c2c11f3f3c80))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.12.1 ([#475](https://www.github.com/googleapis/java-bigtable/issues/475)) ([9e56edf](https://www.github.com/googleapis/java-bigtable/commit/9e56edfa7b0a78f14518a99130a7b319c5873be4))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.13.0 ([#484](https://www.github.com/googleapis/java-bigtable/issues/484)) ([aad648f](https://www.github.com/googleapis/java-bigtable/commit/aad648fec16b122092d394350822da742a2d7aa0))
* update dependency com.google.truth:truth to v1.1 ([#483](https://www.github.com/googleapis/java-bigtable/issues/483)) ([cca1e0e](https://www.github.com/googleapis/java-bigtable/commit/cca1e0e16f2ec0cc887d81c1844f5395ce08b6ea))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 2021
googleapis#474)

Set up channel priming in EnhancedBigtableStubSettings when refreshingChannel is true. BigtableDataClientFactory will set up refreshing channels in the shared client context. Derivative clients cannot reconfigure this setting and will all have refreshing channels.
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.17.0](https://www.github.com/googleapis/java-bigtable/compare/v1.16.2...v1.17.0) (2020-10-23)


### Features

* attemp DirectPath by default ([googleapis#467](https://www.github.com/googleapis/java-bigtable/issues/467)) ([89c622d](https://www.github.com/googleapis/java-bigtable/commit/89c622da6038067892687af3edafae743465eda7))
* backup level IAM ([googleapis#450](https://www.github.com/googleapis/java-bigtable/issues/450)) ([f38a8ec](https://www.github.com/googleapis/java-bigtable/commit/f38a8ecdc6164d081ef96f748ea37bd62b29b419))
* Implement toString for Bigtable*Settings ([googleapis#488](https://www.github.com/googleapis/java-bigtable/issues/488)) ([4d821f8](https://www.github.com/googleapis/java-bigtable/commit/4d821f85ceb237c8e449243ff8c80fb94e22ad51))


### Bug Fixes

* Make refreshing channel compatible with BigtableDataClientFactory ([googleapis#474](https://www.github.com/googleapis/java-bigtable/issues/474)) ([fc74164](https://www.github.com/googleapis/java-bigtable/commit/fc741645536e01fac772136bc8346f73ff95e600))


### Documentation

* fix formatting ([googleapis#476](https://www.github.com/googleapis/java-bigtable/issues/476)) ([eb24569](https://www.github.com/googleapis/java-bigtable/commit/eb24569e53f9d2b7fde50748c840c2c11f3f3c80))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.12.1 ([googleapis#475](https://www.github.com/googleapis/java-bigtable/issues/475)) ([9e56edf](https://www.github.com/googleapis/java-bigtable/commit/9e56edfa7b0a78f14518a99130a7b319c5873be4))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.13.0 ([googleapis#484](https://www.github.com/googleapis/java-bigtable/issues/484)) ([aad648f](https://www.github.com/googleapis/java-bigtable/commit/aad648fec16b122092d394350822da742a2d7aa0))
* update dependency com.google.truth:truth to v1.1 ([googleapis#483](https://www.github.com/googleapis/java-bigtable/issues/483)) ([cca1e0e](https://www.github.com/googleapis/java-bigtable/commit/cca1e0e16f2ec0cc887d81c1844f5395ce08b6ea))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
@mutianf mutianf deleted the issue458 branch Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants