feat: Add experimental DirectPath support #396

WeiranFang · 2020-08-24T23:28:46Z

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

chingor13 · 2020-08-24T23:35:54Z

google-cloud-spanner/pom.xml

@@ -49,6 +51,7 @@
            <id>default-test</id>
            <configuration>
              <excludedGroups>com.google.cloud.spanner.TracerTest,com.google.cloud.spanner.IntegrationTest</excludedGroups>
+              <skipTests>${skipUTs}</skipTests>


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

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?

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.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/GceTestEnvConfig.java

codecov · 2020-08-25T00:01:56Z

Codecov Report

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

@@            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.

thiagotnunes

Thanks for adding this change!

thiagotnunes · 2020-08-27T00:09:42Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java

+            .setHeaderProvider(mergedHeaderProvider);
+
+    // TODO(weiranf): Set to true by default once DirectPath goes to public beta.
+    if (shouldAttemptDirectPath()) {


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?

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?

That makes sense, thanks for the response.

A couple of questions on the approach:

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?

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!

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.

chingor13

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

WeiranFang · 2020-09-09T05:41:04Z

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

thiagotnunes · 2020-09-09T05:48:45Z

@WeiranFang nothing from side. Can you merge?

WeiranFang · 2020-09-09T18:31:56Z

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 · 2020-09-09T23:21:57Z

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

We actually don't use static check in this repo so this is just safe to remove. Fixes: googleapis#354 Updates internal bug: b/155334826

* feat!: upgrade to Java 8 and JDBC 4.2 Upgrade to Java 8 and implement new methods added in Java 8 for JDBC. Drops support for Java 7. Closes googleapis#396 * fix: remove Java 7 from CI * test: add additional test cases * test: add tests for batches * build: remove java7 configs

feat: Add experimental DirectPath support

b6b1428

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2020

chingor13 requested a review from skuruppu August 24, 2020 23:33

chingor13 reviewed Aug 24, 2020

View reviewed changes

product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Aug 25, 2020

skuruppu requested a review from thiagotnunes August 26, 2020 11:28

thiagotnunes reviewed Aug 27, 2020

View reviewed changes

fix lint

01327bf

thiagotnunes approved these changes Aug 28, 2020

View reviewed changes

chingor13 approved these changes Sep 1, 2020

View reviewed changes

thiagotnunes merged commit 46264d1 into googleapis:master Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add experimental DirectPath support #396

feat: Add experimental DirectPath support #396

WeiranFang commented Aug 24, 2020

chingor13 Aug 24, 2020

WeiranFang Aug 27, 2020

chingor13 Sep 1, 2020

codecov bot commented Aug 25, 2020 •

edited

thiagotnunes left a comment

thiagotnunes Aug 27, 2020

WeiranFang Aug 27, 2020

thiagotnunes Aug 28, 2020 •

edited

WeiranFang Aug 28, 2020

chingor13 left a comment

WeiranFang commented Sep 9, 2020

thiagotnunes commented Sep 9, 2020

WeiranFang commented Sep 9, 2020

thiagotnunes commented Sep 9, 2020

feat: Add experimental DirectPath support #396

feat: Add experimental DirectPath support #396

Conversation

WeiranFang commented Aug 24, 2020

chingor13 Aug 24, 2020

Choose a reason for hiding this comment

WeiranFang Aug 27, 2020

Choose a reason for hiding this comment

chingor13 Sep 1, 2020

Choose a reason for hiding this comment

codecov bot commented Aug 25, 2020 • edited

Codecov Report

thiagotnunes left a comment

Choose a reason for hiding this comment

thiagotnunes Aug 27, 2020

Choose a reason for hiding this comment

WeiranFang Aug 27, 2020

Choose a reason for hiding this comment

thiagotnunes Aug 28, 2020 • edited

Choose a reason for hiding this comment

WeiranFang Aug 28, 2020

Choose a reason for hiding this comment

chingor13 left a comment

Choose a reason for hiding this comment

WeiranFang commented Sep 9, 2020

thiagotnunes commented Sep 9, 2020

WeiranFang commented Sep 9, 2020

thiagotnunes commented Sep 9, 2020

codecov bot commented Aug 25, 2020 •

edited

thiagotnunes Aug 28, 2020 •

edited