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
Merged
5 changes: 5 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ const gapic = Object.freeze({
v1: require('./v1'),
});

const gcpApiConfig = require('./spanner_grpc_config.json');

/*!
* DO NOT DELETE THE FOLLOWING NAMESPACE DEFINITIONS
*/
Expand Down Expand Up @@ -206,6 +208,9 @@ class Spanner extends Service {
},
options || {});

// Enable grpc-gcp support
options = Object.assign({'grpc_gcp.apiConfig': gcpApiConfig}, options);

const config = {
baseUrl: options.servicePath || gapic.v1.SpannerClient.servicePath,
protosDir: path.resolve(__dirname, '../protos'),
Expand Down
92 changes: 92 additions & 0 deletions src/spanner_grpc_config.json
Original file line number Diff line number Diff line change
@@ -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?

"channelPool": {
"maxSize": 10,
"maxConcurrentStreamsLowWatermark": 100
},
"method": [
{
"name": [ "/google.spanner.v1.Spanner/CreateSession" ],
"affinity": {
"command": "BIND",
"affinityKey": "name"
}
},
{
"name": [ "/google.spanner.v1.Spanner/GetSession" ],
"affinity": {
"command": "BOUND",
"affinityKey": "name"
}
},
{
"name": [ "/google.spanner.v1.Spanner/DeleteSession" ],
"affinity": {
"command": "UNBIND",
"affinityKey": "name"
}
},
{
"name": [ "/google.spanner.v1.Spanner/ExecuteSql" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/ExecuteStreamingSql" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/Read" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/StreamingRead" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/BeginTransaction" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/Commit" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/Rollback" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/PartitionQuery" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
},
{
"name": [ "/google.spanner.v1.Spanner/PartitionRead" ],
"affinity": {
"command": "BOUND",
"affinityKey": "session"
}
}
]
}
5 changes: 5 additions & 0 deletions test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import * as pfy from '@google-cloud/promisify';
import * as sinon from 'sinon';
import * as spnr from '../src';

const apiConfig = require('../src/spanner_grpc_config.json');
JustinBeckwith marked this conversation as resolved.
Show resolved Hide resolved

function getFake(obj: {}) {
return obj as {
calledWith_: IArguments;
Expand Down Expand Up @@ -187,6 +189,7 @@ describe('Spanner', () => {
libName: 'gccl',
libVersion: require('../../package.json').version,
scopes: [],
'grpc_gcp.apiConfig': apiConfig,
});

assert.deepStrictEqual(
Expand All @@ -205,6 +208,7 @@ describe('Spanner', () => {
libName: 'gccl',
libVersion: require('../../package.json').version,
scopes: expectedScopes,
'grpc_gcp.apiConfig': apiConfig,
});

assert.deepStrictEqual(
Expand Down Expand Up @@ -235,6 +239,7 @@ describe('Spanner', () => {
libName: 'gccl',
libVersion: require('../../package.json').version,
scopes: [],
'grpc_gcp.apiConfig': apiConfig,
}));
});

Expand Down
4 changes: 3 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
"compilerOptions": {
"rootDir": ".",
"outDir": "build",
"noImplicitAny": false
"noImplicitAny": false,
"resolveJsonModule": true
},
"include": [
"src/*.ts",
"src/v1/*.d.ts",
"src/**/*.js",
"src/**/*.json",
"test/*.ts",
"system-test/*.ts",
]
Expand Down