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: Add experimental DirectPath support #396

Merged
merged 2 commits into from Sep 9, 2020

Conversation

WeiranFang
Copy link
Contributor

@WeiranFang WeiranFang commented Aug 24, 2020

Previously DirectPath was enabled by an environment variable. Now since gax-java v1.57.0 DirectPath can be enabled by using InstantiatingGrpcChannelProvider.Builder.setAttemptDirectPath(true).

Some changes are temporary and will be removed once DirectPath goes public using the default spanner endpoint. For now we want to be able to test the DirectPath-enabled endpoint with different client scenarios:

  • Client is running on a ipv4-only VM
  • Client is running on a ipv6-enabled VM
  • Client fallbacks to use normal CFE path

For the purpose of our internal E2E tests, I added a test profile called spanner-directpath-it which introduces two new sys props:

  • spanner.attempt_directpath: this defines whether the test client would call setAttemptDirectPath(true) and set the spanner endpoint to use the DirectPath-enabled target aa423245250f2bbf.sandbox.googleapis.com:443.
  • spanner.directpath_test_scenario: this defines which of the three scenarios mentioned above to test.

And for different scenarios we would have different assertions for the test:

  • For ipv4-only VM, we introspect the peer IP addresses and expect they have DP_IPV4_PREFIX.
  • For ipv6-enabled VM, we introspect the peer IP addresses and expect they have either DP_IPV4_PREFIX or DP_IPV6_PREFIX.
  • For fallback cases, we should allow both DirectPath addresses and CFE addresses.

+cc @chingor13 , @vnorigoog

@google-cla google-cla bot added the cla: yes label Aug 24, 2020
@chingor13 chingor13 requested a review from skuruppu Aug 24, 2020
@@ -49,6 +51,7 @@
<id>default-test</id>
<configuration>
<excludedGroups>com.google.cloud.spanner.TracerTest,com.google.cloud.spanner.IntegrationTest</excludedGroups>
<skipTests>${skipUTs}</skipTests>
Copy link
Contributor

@chingor13 chingor13 Aug 24, 2020

Choose a reason for hiding this comment

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

I only see this being set to false. Do we need this configuration?

Copy link
Contributor Author

@WeiranFang WeiranFang Aug 27, 2020

Choose a reason for hiding this comment

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

Hi Jeff, I added this configuration just for our internal test to be able to use -DskipUTs=true. Currently we are using mvn verify -am -pl google-cloud-spanner -B -Penable-integration-tests,spanner-directpath-it -DskipUTs=true -D... ... for our internal E2E DP tests, and meanwhile I don't want to change the behavior for other tests. Do you think this is the right way for approaching this purpose?

Copy link
Contributor

@chingor13 chingor13 Sep 1, 2020

Choose a reason for hiding this comment

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

I might add a note somewhere so we don't inadvertently break this.

We might also want to add something like this to our shared setup.

@codecov
Copy link

@codecov codecov bot commented Aug 25, 2020

Codecov Report

No coverage uploaded for pull request base (master@bd5c93f). Click here to learn what that means.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #396   +/-   ##
=========================================
  Coverage          ?   82.08%           
  Complexity        ?     2451           
=========================================
  Files             ?      136           
  Lines             ?    13587           
  Branches          ?     1306           
=========================================
  Hits              ?    11153           
  Misses            ?     1906           
  Partials          ?      528           
Impacted Files Coverage Δ Complexity Δ
...m/google/cloud/spanner/spi/v1/GapicSpannerRpc.java 82.03% <90.90%> (ø) 81.00 <0.00> (?)
...gle/cloud/spanner/v1/stub/SpannerStubSettings.java 94.57% <0.00%> (ø) 26.00% <0.00%> (?%)
...c/main/java/com/google/cloud/spanner/BackupId.java 75.00% <0.00%> (ø) 12.00% <0.00%> (?%)
.../cloud/spanner/connection/StatementResultImpl.java 100.00% <0.00%> (ø) 20.00% <0.00%> (?%)
...m/google/cloud/spanner/ForwardingStructReader.java 23.23% <0.00%> (ø) 11.00% <0.00%> (?%)
...src/main/java/com/google/cloud/spanner/Struct.java 88.67% <0.00%> (ø) 28.00% <0.00%> (?%)
...oogle/cloud/spanner/spi/v1/LoggingInterceptor.java 14.70% <0.00%> (ø) 2.00% <0.00%> (?%)
...oud/spanner/connection/AbstractBaseUnitOfWork.java 84.12% <0.00%> (ø) 8.00% <0.00%> (?%)
...ogle/cloud/spanner/connection/StatementParser.java 86.63% <0.00%> (ø) 50.00% <0.00%> (?%)
...cloud/spanner/connection/ReadWriteTransaction.java 62.37% <0.00%> (ø) 44.00% <0.00%> (?%)
... and 127 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 bd5c93f...01327bf. Read the comment docs.

@product-auto-label product-auto-label bot added the api: spanner label Aug 25, 2020
@skuruppu skuruppu requested a review from thiagotnunes Aug 26, 2020
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Thanks for adding this change!

.setHeaderProvider(mergedHeaderProvider);

// TODO(weiranf): Set to true by default once DirectPath goes to public beta.
if (shouldAttemptDirectPath()) {
Copy link
Contributor

@thiagotnunes thiagotnunes Aug 27, 2020

Choose a reason for hiding this comment

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

I think that currently we are mostly enabling features / configurations through environment variables (which should be standardised through all of our clients) or through the SpannerOptions class.

I imagine we could add an option like useDirectPath() in the SpannerOptions.Builder class. I guess that even if we do not use an environment variable as well (and go with the property), it would be nice to concentrated the configuration directly in the SpannerOptions class.

Wdyt?

Copy link
Contributor Author

@WeiranFang WeiranFang Aug 27, 2020

Choose a reason for hiding this comment

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

Hi Thiago, thanks for the comment! So our original idea is, the DirectPath is a network side feature, it should be agnostic to the end users. Currently we add this shouldAttemptDirectPath temporarily so our test can start exercising DirectPath. Once DirectPath is going public, we will remove these temporary checker and set the client to use DirectPath by default. And correct me if I'm wrong, this SpannerOptions seems like a configurable option provided to end users, but in fact we don't actually want users to config DirectPath attempt, which should only be controlled by service owner/SREs via Access Control List. WDYT?

Copy link
Contributor

@thiagotnunes thiagotnunes Aug 28, 2020

Choose a reason for hiding this comment

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

That makes sense, thanks for the response.

A couple of questions on the approach:

  1. Aren't we enabling this to a few customers at first (before being the default)? If so, is the expectation that they would have to execute with the property set?
  2. Should we override the endpoint even when the user provides a custom channel provider? I don't know what is best here, but I imagine that it might be confusing if the user executes their program with the property set and a custom channel provider, but do not get the benefits of the direct path.

Thanks!

Copy link
Contributor Author

@WeiranFang WeiranFang Aug 28, 2020

Choose a reason for hiding this comment

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

Good questions! To answer them:
Actually we would need to remove these temporary checker and enable DirectPath with a client lib release before we engage real customers, because DirectPath should be fully controlled by the ACL config (i.e. initially only a few customers will get ALLOW from ACL check, all others will get DENY). So this property set should only be used by our tests.

This sandbox endpoint is only used for our testing purposes (as right now DirectPath-ipv4 is only available in certain cell in prod) Once we've done enough tests and DirectPath is fully ready, Spanner will make its default endpoint to be capable of handling DirectPath traffic. So by that time, we will use the same endpoint no matter the client is using DirectPath or Not.

Copy link
Contributor

@chingor13 chingor13 left a comment

I'm good with these changes if the spanner team is

@WeiranFang
Copy link
Contributor Author

@WeiranFang WeiranFang commented Sep 9, 2020

Hi @thiagotnunes and @chingor13, is there anything else need to be done before this can be merge?

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Sep 9, 2020

@WeiranFang nothing from side. Can you merge?

@WeiranFang
Copy link
Contributor Author

@WeiranFang WeiranFang commented Sep 9, 2020

Hi @thiagotnunes Actually I don't have access to merge this PR:

"Only those with write access to this repository can merge pull requests."

is it OK to add me write access to this repo? Thanks!

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Sep 9, 2020

@WeiranFang you are right, I will merge it for you and follow up if I can give you write access.

@thiagotnunes thiagotnunes merged commit 46264d1 into googleapis:master Sep 9, 2020
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants