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

Add spanner_grpc_config.json and enable grpc-gcp support for spanner #503

Merged
merged 13 commits into from
Feb 15, 2019

Conversation

WeiranFang
Copy link
Contributor

Hi @JustinBeckwith @alexander-fenster,

This is to enable channel pool support for spanner (from previous discussions in #436).

The channel pooling won't be enabled until googleapis/gax-nodejs#396 is merged and released.

Question: Once gax changes are merged and released, do I pin the new version in this PR or create another PR just for the gax version bump?

Thanks

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 29, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

I leave this one to @crwilcox :)

@@ -0,0 +1,92 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this all essentially fine tuning for each API endpoint we access? Who is supposed to keep this up to date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it's configured as this to address the max 100 concurrent streams issue, but I think as for who's responsible for future changes in this file, it depends on which parts to change. For example if the affinity key changes, then the service owner needs to change it, and if the grpc channel max throughput changes, then we should make the changes accordingly. Does that sound reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

My one thought here is if there is a way to combine this with our other configuration at some point. We already specify retry conditions for rpc calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crwilcox We already maintain a general ApiConfig proto definition for all languages, so I believe it would be good to have a separate config file specifically for this proto. Also FWIK in future more grpc features (like retry logic and regional support) will also be supported in this grpc-gcp library. In that case those grpc related configs can all be put into this config file.. WDYT?

test/index.ts Show resolved Hide resolved
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 5, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 5, 2019
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2019
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 11, 2019
@WeiranFang
Copy link
Contributor Author

Hi @JustinBeckwith , I noticed that there are some tests failed for grpc-js system test. grpc-js is the new pure js grpc package, which currently doesn't support our grpc-gcp extension. Can we ignore the grpc-js test for now for spanner?

@alexander-fenster
Copy link
Contributor

@WeiranFang These tests now fail for many packages, not only for Spanner and not only because of the unsupported options. I'm going to eventually make gax work with grpc-js without failures since we'll eventually move from grpc to grpc-js. By that time, you'll need to get the support for your features ported to grpc-js. We expect that to happen around Q2.

Until that time, it's OK to ignore those grpc-js failures. That's why that check is not marked Required in GitHub :)

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #503 into master will increase coverage by 0.39%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
+ Coverage   89.75%   90.14%   +0.39%     
==========================================
  Files          15       15              
  Lines        1415     1411       -4     
  Branches       42       42              
==========================================
+ Hits         1270     1272       +2     
+ Misses        140      134       -6     
  Partials        5        5
Impacted Files Coverage Δ
src/index.ts 99.08% <100%> (+0.01%) ⬆️
src/v1/spanner_client.js 5.94% <0%> (+0.11%) ⬆️
src/v1/instance_admin_client.js 83.16% <0%> (+1.61%) ⬆️
src/v1/database_admin_client.js 83.87% <0%> (+1.76%) ⬆️

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 e770252...6d2b428. Read the comment docs.

@WeiranFang
Copy link
Contributor Author

Thanks @alexander-fenster for letting me know :) And is there anything I should do before you merge this PR?

@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 15, 2019
@JustinBeckwith JustinBeckwith merged commit 7cfe40f into googleapis:master Feb 15, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants