FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Bigtable: make client settings more consistent by igorbernstein2 · Pull Request #4498 · googleapis/google-cloud-java · GitHub

/ google-cloud-java Public

Bigtable: make client settings more consistent - #4498

Closed
igorbernstein2 wants to merge 7 commits into
googleapis:masterfrom
igorbernstein2:settings
ConversationCommits (7)ChecksFiles changed
Closed

Bigtable: make client settings more consistent#4498
igorbernstein2 wants to merge 7 commits into
googleapis:masterfrom
igorbernstein2:settings

Conversation

igorbernstein2 commented Feb 14, 2019

Copy link
Copy Markdown
Contributor

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)

Bigtable: make client settings more consistent
78dcf3c 
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 requested a review from a team February 14, 2019 21:32
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 14, 2019
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 removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019

codecov Bot commented Feb 14, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@             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 commented Feb 15, 2019

Copy link
Copy Markdown
Contributor

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

Merge remote-tracking branch 'origin/master' into settings
8adb4c8 
# Conflicts:
#	google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/ResourceHeaderTest.java

igorbernstein2 commented Feb 15, 2019

Copy link
Copy Markdown
Contributor Author

I think it was just a merge conflict

sduskis reviewed Feb 15, 2019
View reviewed changes

sduskis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

This was referenced Feb 15, 2019
Merged
Merged
Merged
Merged
Merged

igorbernstein2 commented Feb 15, 2019

Copy link
Copy Markdown
Contributor Author

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

khakha010 reviewed Feb 21, 2019
View reviewed changes

khakha010 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

`

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
sduskis sduskis left review comments
khakha010 khakha010 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BigtableTableAdminSettings is missing credential provider options

5 participants

Back | FazBrowse Home | New Git URL