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

Bigtable: make client settings more consistent #4498

Closed
wants to merge 7 commits into from

Conversation

igorbernstein2
Copy link

The eventual goal is to deemphasize per method RPC settings by relegating them to stubSettings. And promote more common settings like credentials to the top level.

This breaks backwards compatibility by removing BigtableDataSettings' base class. It tries to ease the transition by copying the ClientSettings methods down into BigtableDataSettings.

Specific changes:

  • BigtableDataSettings is no longer a subclass of ClientSettings
  • ClientSettings methods have been temporarily copied to BigtableDataSettings with deprecation warnings to ease transition
  • add helper method to ease connection to emulator
  • make sure that credential provider is configurable at the top level (fixes BigtableTableAdminSettings is missing credential provider options  #3634)
  • all of the deprecated methods have been grouped together and will be removed in the next couple of releases (along with the deprecated google-bigtable-admin artifact)

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for context and/or discussion)

The eventual goal is to deemphasize per method RPC settings by relegating them to stubSettings. And promote more common settings like credentials to the top level.

This breaks backwards compatibility by removing BigtableDataSettings' base class. It tries to ease the transition by copying the ClientSettings methods down into BigtableDataSettings.

Specific changes:
- BigtableDataSettings is no longer a subclass of ClientSettings
- ClientSettings methods have been temporarily copied to BigtableDataSettings with deprecation warnings to ease transition
- add helper method to ease connection to emulator
- make sure that credential provider is configurable at the top level (fixes googleapis#3634)
- all of the deprecated methods have been grouped together and will be removed in the next couple of releases (along with the deprecated google-bigtable-admin artifact)
@igorbernstein2 igorbernstein2 requested a review from a team as a code owner February 14, 2019 21:32
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 14, 2019
@igorbernstein2 igorbernstein2 added kokoro:force-run Add this label to force Kokoro to re-run the tests. api: bigtable Issues related to the Bigtable API. labels Feb 14, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #4498 into master will decrease coverage by 0.01%.
The diff coverage is 16.4%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4498      +/-   ##
============================================
- Coverage     49.16%   49.15%   -0.02%     
- Complexity    21915    21917       +2     
============================================
  Files          2078     2078              
  Lines        207084   207152      +68     
  Branches      24084    24084              
============================================
+ Hits         101815   101818       +3     
- Misses        97126    97191      +65     
  Partials       8143     8143
Impacted Files Coverage Δ Complexity Δ
...loud/bigtable/data/v2/internal/RequestContext.java 100% <ø> (+66.66%) 2 <0> (ø) ⬇️
...gtable/admin/v2/BigtableInstanceAdminSettings.java 74.19% <0%> (-7.95%) 5 <0> (ø)
.../bigtable/admin/v2/BigtableTableAdminSettings.java 54.71% <10.52%> (-23.67%) 6 <0> (ø)
...gle/cloud/bigtable/data/v2/BigtableDataClient.java 84.31% <100%> (ø) 32 <1> (ø) ⬇️
...e/cloud/bigtable/data/v2/BigtableDataSettings.java 17.02% <11.9%> (-14.69%) 3 <2> (ø)
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 93.19% <46.66%> (ø) 17 <3> (+2) ⬆️

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 228192e...dfe564c. Read the comment docs.

@sduskis
Copy link
Contributor

sduskis commented Feb 15, 2019

@igorbernstein2, it looks like all tests are failing now.

# Conflicts:
#	google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/ResourceHeaderTest.java
@igorbernstein2
Copy link
Author

I think it was just a merge conflict

Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

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

There are a lot of different threads of work here. Can you please break down this work into smaller PRs so that the reviewing can be more effective?

@igorbernstein2
Copy link
Author

Ok, I closing in favor of #4505, #4506, #4507, #4508 and #4509

Copy link

@khakha010 khakha010 left a comment

Choose a reason for hiding this comment

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

`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigtableTableAdminSettings is missing credential provider options
5 participants