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

feat: attemp DirectPath by default #467

Merged
merged 2 commits into from Oct 19, 2020

Conversation

WeiranFang
Copy link
Contributor

@WeiranFang WeiranFang commented Oct 14, 2020

Update client to attempt DirectPath by default, and remove DirectPath test endpoint.

Note that it doesn't mean that after this change client will just use DirectPath, but will call the DirectPath codepath by default.

The actually enablement of DirectPath is controlled by service owner via ACL config. For now, after this change, although all users will attempt DirectPath, but they will all just fallback to the original CFE path.

For testing, we will just use -Dbigtable.directpath-data-endpoint and -Dbigtable.directpath-admin-endpoint to override the default endpoint with DP enabled endpoints.

@igorbernstein2
@mgarolera
@mohanli-ml

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

@codecov codecov bot commented Oct 14, 2020

Codecov Report

Merging #467 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #467      +/-   ##
============================================
+ Coverage     80.65%   80.68%   +0.02%     
+ Complexity     1115     1114       -1     
============================================
  Files           105      105              
  Lines          6941     6936       -5     
  Branches        369      367       -2     
============================================
- Hits           5598     5596       -2     
  Misses         1144     1144              
+ Partials        199      196       -3     
Impacted Files Coverage Δ Complexity Δ
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 98.65% <100.00%> (+1.72%) 21.00 <0.00> (-1.00) ⬆️
...om/google/cloud/bigtable/emulator/v2/Emulator.java 59.83% <0.00%> (-0.82%) 14.00% <0.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 647f926...d9bfde2. Read the comment docs.

@product-auto-label product-auto-label bot added the api: bigtable label Oct 15, 2020
@@ -166,12 +163,6 @@
private EnhancedBigtableStubSettings(Builder builder) {
super(builder);

if (DIRECT_PATH_ENDPOINT.equals(builder.getEndpoint())) {
logger.warning(
"Connecting to Bigtable using DirectPath."
Copy link
Collaborator

@kolea2 kolea2 Oct 15, 2020

Choose a reason for hiding this comment

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

is there anywhere we can keep this logging information to confirm (in tests, for example) that we are connecting using DirectPath?

Copy link
Contributor

@igorbernstein2 igorbernstein2 Oct 15, 2020

Choose a reason for hiding this comment

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

For tests we inject hooks into netty to ensure that directpath is used, also the integration tests generate verbose grpc logs which would show directpath usage. For prod usage, we dont really have a good way to introspect it as its a grpc level feature

// TODO(weiranf): Remove this once DirectPath goes to public beta
private static boolean isDirectPathEnabled() {
return Boolean.getBoolean("bigtable.attempt-directpath");
.setAttemptDirectPath(true);

Choose a reason for hiding this comment

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

Can you add a comment on why we attemt DirectPath? to give context to future readers.

Copy link
Contributor Author

@WeiranFang WeiranFang Oct 15, 2020

Choose a reason for hiding this comment

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

Added comment. PTAL at the wording, thanks!

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

lgtm

mgarolera
Copy link

@mgarolera mgarolera commented on d9bfde2 Oct 15, 2020

Choose a reason for hiding this comment

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

LGTM

@igorbernstein2 igorbernstein2 merged commit 89c622d into googleapis:master Oct 19, 2020
20 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).
WeiranFang added a commit to WeiranFang/java-bigtable that referenced this issue Nov 10, 2020
kolea2 pushed a commit that referenced this issue Nov 10, 2020
@release-please release-please bot mentioned this pull request Nov 10, 2020
mutianf pushed a commit to mutianf/java-bigtable that referenced this issue Nov 12, 2020
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 2021
* feat: attemp DirectPath by default

* Add comment for DP attempt
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).
ad548 pushed a commit to ad548/java-bigtable that referenced this issue Mar 13, 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.

None yet

4 participants