feat(spanner) : add support for client resource base routing #4548

jiren · 2019-12-30T14:35:22Z

Towards #4547 issue

Implementation of Client resource based routing.

Route requests based on available instance endpoint uri. Enable/Disable resource based routing using ENV variable(GOOGLE_CLOUD_SPANNER_ENABLE_RESOURCE_BASED_ROUTING=TRUE) or manual flag enable_resource_based_routing while creating a database client instance . Default client resource based routing is disabled.

Implementation overview

Get instance API with field mask
If resource based routing enabled create client connection using instance endpoint uris.
- Get instance endpoint uris
  - If endpoint uris present, then create a connection using the first uri and cached connection into client instance.
  - If endpoint uris does not present then use default connection with global spanner endpoint uri

hengfengli · 2020-01-14T03:24:28Z

@jiren Could you please fix the CI check error? After that, we can review this PR. Thanks.

jiren · 2020-01-18T12:13:32Z

@jiren Could you please fix the CI check error? After that, we can review this PR. Thanks.

@hengfengli CI error fixed for OS X.

hengfengli

My major concern is whether we need to add ClientServiceProxy and update service.rb.

I think in client.rb::initialize, we can have the following logic:

if resource-based routing is enabled,
- the returned endpoint is empty: use the project.service.
- the returned endpoint is same as the given one in the project.service: use the project.service.
- the returned endpoint is different: we can create a service with the instance-specific endpoint and store it in the client instance.
If resource-based routing is not enabled, then we just use the project.service.

In this case, we don't need to update service.rb.

google-cloud-spanner/acceptance/spanner/client_test.rb

google-cloud-spanner/acceptance/spanner_helper.rb

google-cloud-spanner/lib/google/cloud/spanner/batch_client.rb

google-cloud-spanner/lib/google/cloud/spanner/client.rb

google-cloud-spanner/lib/google/cloud/spanner/service.rb

google-cloud-spanner/lib/google/cloud/spanner/client.rb

google-cloud-spanner/lib/google/cloud/spanner/session.rb

google-cloud-spanner/test/google/cloud/spanner/client_service_proxy/endpont_uri_test.rb

google-cloud-spanner/test/google/cloud/spanner/service_test.rb

jiren · 2020-01-22T04:29:00Z

@hengfengli I will fix the conflict once we finalize service object creation in the client, due to channel-related changes in service.rb, current resource-based routing code will not work

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb

google-cloud-spanner/lib/google/cloud/spanner/service.rb

hengfengli · 2020-01-22T23:46:47Z

@jiren Thanks for fixing the issues.

@skuruppu It looks good to me. Please take a look or request someone to take another look.

hengfengli · 2020-01-22T23:51:14Z

@jiren Why does the CI check not run for this PR again? Maybe because of the force-push. Can you try to re-trigger it?

skuruppu

Looks good. Just want all the comments to be addressed before I approve.

google-cloud-spanner/lib/google/cloud/spanner/project.rb

google-cloud-spanner/acceptance/spanner/client_test.rb

google-cloud-spanner/test/google/cloud/spanner/client/resource_based_routing_test.rb

- add/updates docs and test

- instance get method with fields mask

- add unit test for client service proxy

- tests and doc changes

google-cloud-spanner/acceptance/spanner/instance_test.rb

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb

google-cloud-spanner/test/google/cloud/spanner/client/resource_based_routing_test.rb

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb

skuruppu

Thanks for working on this @jiren. LGTM

hengfengli

LGTM.

skuruppu · 2020-01-28T01:18:54Z

One more request @jiren. Please be more descriptive in the PR description. Follow googleapis/google-cloud-python#10183 as an example.

jiren · 2020-01-28T01:55:41Z

One more request @jiren. Please be more descriptive in the PR description. Follow googleapis/google-cloud-python#10183 as an example.

Got it

google-cloud-spanner/lib/google/cloud/spanner/project.rb

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb

skuruppu · 2020-04-23T09:23:14Z

We have decided to implement this functionality on the server side so we no longer need to add this support on the client side.

jiren requested a review from skuruppu December 30, 2019 14:35

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 30, 2019

skuruppu requested a review from mbril January 1, 2020 22:43

jiren self-assigned this Jan 8, 2020

skuruppu requested review from rhiro and hengfengli January 13, 2020 03:12

jiren added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 18, 2020

jiren added the api: spanner Issues related to the Spanner API. label Jan 21, 2020

hengfengli requested changes Jan 22, 2020

View reviewed changes

jiren force-pushed the spanner-resource-base-routing-client branch from 5c42c3f to 521ec84 Compare January 22, 2020 12:17

hengfengli reviewed Jan 22, 2020

View reviewed changes

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb Outdated Show resolved Hide resolved

hengfengli reviewed Jan 22, 2020

View reviewed changes

google-cloud-spanner/lib/google/cloud/spanner/service.rb Outdated Show resolved Hide resolved

hengfengli reviewed Jan 22, 2020

View reviewed changes

google-cloud-spanner/lib/google/cloud/spanner/service.rb Outdated Show resolved Hide resolved

hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2020

skuruppu reviewed Jan 23, 2020

View reviewed changes

jiren added 8 commits January 23, 2020 06:59

add get_instance field_mask

fe71ad9

- add/updates docs and test

instance endpoint_uris wrapper

e99fdee

add client service proxy class for resource base routing

b7e9d16

- instance get method with fields mask

update client and batch client docs and doctests

deedf54

fix field mask param name for get instance

a2c9c93

- add unit test for client service proxy

unit and acceptance test cases for database service client

44c7005

add warning message if permission denied for get endpoint uri

acb179b

add test cases for get instance permission denied

efb233a

jiren added 2 commits January 23, 2020 06:59

removed client_service_proxy and updated client, batch client

af73fe2

- tests and doc changes

fix docs and updated test cases, reformat service code

13eb5cd

jiren force-pushed the spanner-resource-base-routing-client branch from 521ec84 to 13eb5cd Compare January 23, 2020 01:39

fix doc

253f59a

rhiro reviewed Jan 24, 2020

View reviewed changes

fix typos and changes in instance get permission denied test

1fc45f0

skuruppu approved these changes Jan 27, 2020

View reviewed changes

hengfengli approved these changes Jan 27, 2020

View reviewed changes

rhiro approved these changes Jan 30, 2020

View reviewed changes

rhiro removed the request for review from mbril February 4, 2020 23:25

rhiro suggested changes Feb 7, 2020

View reviewed changes

google-cloud-spanner/lib/google/cloud/spanner/project.rb Outdated Show resolved Hide resolved

google-cloud-spanner/lib/google/cloud/spanner/project.rb Outdated Show resolved Hide resolved

google-cloud-spanner/lib/google/cloud/spanner/resource_based_routing.rb Outdated Show resolved Hide resolved

jiren added 3 commits February 8, 2020 10:52

update resource based routing env variable name

d207e43

fix test case

83bbd8c

fix test cases

6af2a4d

skuruppu added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 13, 2020

skuruppu closed this Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner) : add support for client resource base routing #4548

feat(spanner) : add support for client resource base routing #4548

jiren commented Dec 30, 2019 •

edited

hengfengli commented Jan 14, 2020 •

edited

jiren commented Jan 18, 2020

hengfengli left a comment •

edited

jiren commented Jan 22, 2020

hengfengli commented Jan 22, 2020

hengfengli commented Jan 22, 2020

skuruppu left a comment

skuruppu left a comment

hengfengli left a comment

skuruppu commented Jan 28, 2020

jiren commented Jan 28, 2020

skuruppu commented Apr 23, 2020

feat(spanner) : add support for client resource base routing #4548

feat(spanner) : add support for client resource base routing #4548

Conversation

jiren commented Dec 30, 2019 • edited

Implementation of Client resource based routing.

Implementation overview

hengfengli commented Jan 14, 2020 • edited

jiren commented Jan 18, 2020

hengfengli left a comment • edited

Choose a reason for hiding this comment

jiren commented Jan 22, 2020

hengfengli commented Jan 22, 2020

hengfengli commented Jan 22, 2020

skuruppu left a comment

Choose a reason for hiding this comment

skuruppu left a comment

Choose a reason for hiding this comment

hengfengli left a comment

Choose a reason for hiding this comment

skuruppu commented Jan 28, 2020

jiren commented Jan 28, 2020

skuruppu commented Apr 23, 2020

jiren commented Dec 30, 2019 •

edited

hengfengli commented Jan 14, 2020 •

edited

hengfengli left a comment •

edited