feat: add support for spanner client resource base routing #780

laljikanjareeya · 2019-12-31T16:09:06Z

Implement resource based routing.

When preparing Gapic Request for a SpannerClient, If Client code does not specify the apiEndpoint than check for suggested endpointUris.

If endpointUris are returned, use the first one as apiEndpoint.
In case of no endpointUris are returned, use default apiEndpoint.

Gapic Clients are cached per instance on the SpannerClient.

This feature is disabled by default and is enabled by setting the environment variable to true.

Environment variable to enable this feature:
GOOGLE_CLOUD_SPANNER_ENABLE_RESOURCE_BASED_ROUTING

Closes : #779

fix the system test and unit test

… not

fix the existing test cases pass the formattedName_ instead of instanceId and check the instanceId on one place

disable the resource based routing while system test

improve the unit test cases

fix the related changes pending the test case

src/index.ts

src/instance.ts

codecov · 2019-12-31T17:13:25Z

Codecov Report

Merging #780 into master will not change coverage.
The diff coverage is 92.59%.

@@           Coverage Diff           @@
##           master     #780   +/-   ##
=======================================
  Coverage   76.74%   76.74%           
=======================================
  Files          43       43           
  Lines       20467    20467           
  Branches      597      597           
=======================================
  Hits        15707    15707           
  Misses       4753     4753           
  Partials        7        7

Impacted Files	Coverage Δ
src/session-pool.ts	`99.26% <100%> (ø)`	⬆️
src/transaction-runner.ts	`98% <87.5%> (ø)`	⬆️
src/database.ts	`99.77% <93.75%> (ø)`	⬆️

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 a464043...f3b976c. Read the comment docs.

src/index.ts

test/index.ts

system-test/spanner.ts

src/session.ts

system-test/spanner.ts

skuruppu

There are still some unresolved comments so I will wait for you to resolve them before approving.

test/index.ts

system-test/spanner.ts

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

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

olavloite · 2020-01-28T08:07:01Z

@skuruppu Regarding #780 (comment) on the minimum number of sessions in the pool: The Nodejs client currently still uses MinSessions=0 as the default value for the session pool.

See

nodejs-spanner/src/session-pool.ts

Line 128 in f9af78a

* @property {number} [min=0] Minimum number of resources to keep in the pool at

skuruppu · 2020-01-29T03:34:52Z

@skuruppu Regarding #780 (comment) on the minimum number of sessions in the pool: The Nodejs client currently still uses MinSessions=0 as the default value for the session pool.

See

nodejs-spanner/src/session-pool.ts

Line 128 in f9af78a

* @property {number} [min=0] Minimum number of resources to keep in the pool at

Oh got it. Thanks for pointing this out @olavloite.

skuruppu · 2020-02-13T02:53:31Z

system-test/spanner.ts

+            done(); //no resolved endpoint.
+          }
+          //user specific apiEndpoint
+          spanner.options.apiEndpoint = resolvedEndpoint[0];


Isn't this using the resolved endpoint instead of a user-specified endpoint?

skuruppu · 2020-02-13T02:54:38Z

system-test/spanner.ts

+          }
+          //user specific apiEndpoint
+          spanner.options.apiEndpoint = resolvedEndpoint[0];
+          spanner.options.enableResourceBasedRouting = true;


I think at line 4417, resource routing is already enabled so don't think you need this.

skuruppu · 2020-04-23T09:21:34Z

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

Lalji Kanjareeya added 15 commits December 31, 2019 21:30

feat: added method to get endPointUris and related changes

3b5c4de

fix: unit test cases was failing

241fe62

pass instanceId externally in each SpannerClient Request

d4d50b7

fix the system test and unit test

feat: added flag to check weather resource based routing is enable or…

87fd04a

… not

move getInstanceEndPointUris to instance.ts file

1ba9e16

fix the existing test cases pass the formattedName_ instead of instanceId and check the instanceId on one place

test: added unit test cases

2ac95bf

fix: removed end point uris mapping chaching

f504666

disable the resource based routing while system test

test:added more unit test cases

cc5e72f

provided an option to enable resource based routing to user

dfb8100

improve the unit test cases

pass the instance id insted of formmatedName_ in each method

9d4f4af

fix the related changes pending the test case

fix: unit test cases

67f74af

fix: unit test cases

15f261d

test:added system test

fe0e1a5

test:added unit test for instance permission

d46a13c

fix:review changes

c4c12d2

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

laljikanjareeya changed the title ~~feat : add support for spanner client resource base routing~~ feat: add support for spanner client resource base routing Dec 31, 2019

AVaksman reviewed Dec 31, 2019

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

AVaksman reviewed Dec 31, 2019

View reviewed changes

src/instance.ts Outdated Show resolved Hide resolved

AVaksman reviewed Dec 31, 2019

View reviewed changes

src/instance.ts Outdated Show resolved Hide resolved

refactor: simplify and clean up

aae7311

fix: reverse the resource based routing condition

0b7e83e

laljikanjareeya force-pushed the resource-based-routing branch from 6ac1e29 to 0b7e83e Compare January 1, 2020 06:38

Lalji Kanjareeya added 3 commits January 2, 2020 10:56

test: added unit test for multiple endpointUri

59cdb4f

Merge branch 'master' into resource-based-routing

06fba33

fix:correct spell

600d5f1

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

skuruppu suggested changes Jan 21, 2020

View reviewed changes

Lalji Kanjareeya and others added 2 commits January 22, 2020 17:46

Merge branch 'master' into resource-based-routing

1e83f37

Merge branch 'master' into resource-based-routing

57687ba

rhiro reviewed Jan 24, 2020

View reviewed changes

src/session.ts Show resolved Hide resolved

system-test/spanner.ts Show resolved Hide resolved

Lalji Kanjareeya added 4 commits January 24, 2020 15:24

Merge branch 'master' into resource-based-routing

eefc95d

fix:review changes

4c2f6aa

fix: unit-test case fail issue

48085fb

refactor: added enum to check the permission status

eb358f3

skuruppu reviewed Jan 28, 2020

View reviewed changes

test/index.ts Outdated Show resolved Hide resolved

system-test/spanner.ts Outdated Show resolved Hide resolved

Merge branch 'master' into resource-based-routing

06dc72d

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

skuruppu approved these changes Feb 6, 2020

View reviewed changes

Lalji Kanjareeya added 2 commits February 6, 2020 10:24

merge:merge from master

e5b02c5

fix: review changes

710bc9a

laljikanjareeya added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020

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

Lalji Kanjareeya added 3 commits February 10, 2020 10:51

Merge branch 'master' into resource-based-routing

66eac38

test: added unit test and system-test cases

a464043

Merge branch 'master' into resource-based-routing

f3b976c

laljikanjareeya requested review from rhiro and AVaksman February 12, 2020 05:44

laljikanjareeya added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020

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

skuruppu reviewed Feb 13, 2020

View reviewed changes

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: add support for spanner client resource base routing #780

feat: add support for spanner client resource base routing #780

laljikanjareeya commented Dec 31, 2019 •

edited

codecov bot commented Dec 31, 2019 •

edited

skuruppu left a comment

skuruppu commented Jan 28, 2020

olavloite commented Jan 28, 2020

skuruppu commented Jan 29, 2020

skuruppu Feb 13, 2020

skuruppu Feb 13, 2020

skuruppu commented Apr 23, 2020

feat: add support for spanner client resource base routing #780

feat: add support for spanner client resource base routing #780

Conversation

laljikanjareeya commented Dec 31, 2019 • edited

codecov bot commented Dec 31, 2019 • edited

Codecov Report

skuruppu left a comment

Choose a reason for hiding this comment

skuruppu commented Jan 28, 2020

olavloite commented Jan 28, 2020

skuruppu commented Jan 29, 2020

skuruppu Feb 13, 2020

Choose a reason for hiding this comment

skuruppu Feb 13, 2020

Choose a reason for hiding this comment

skuruppu commented Apr 23, 2020

laljikanjareeya commented Dec 31, 2019 •

edited

codecov bot commented Dec 31, 2019 •

edited