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
feat: Multi cluster routing #1007
feat: Multi cluster routing #1007
Conversation
…into multi-cluster-routing # Conflicts: # test/instance.ts
system-test/app-profile.ts
Outdated
.slice(1, 3) | ||
.map(clusterId => instance.cluster(clusterId)); | ||
const options = { | ||
routing: clusterSubset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine (and a little easier to read) if you just hardcode 2/3 of the cluster ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll go make this change.
import {Bigtable} from '../src'; | ||
import assert = require('assert'); | ||
|
||
describe('📦 App Profile', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great start! Let's add a few more tests
- create an app profile with single cluster routing
- create an app profile with multi cluster routing (no cluster ids)
- create an app profile with single cluster routing and update it to multi cluster routing with ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these tests. In the process of adding these tests it looks like a lot of restructuring was required in order to eliminate duplicate code fragments so let me know if you have any questions regarding the changes.
import {Cluster} from '../src/cluster'; | ||
import * as inst from '../src/instance'; | ||
|
||
export const PREFIX = 'gcloud-tests-'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this cleanup from somewhere else? Should it be removed there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables were moved into a file called common.js because now two files use them. app-profile.ts
is one of them and the other is instance.ts
.
|
||
export const PREFIX = 'gcloud-tests-'; | ||
|
||
export function generateId(resourceType: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables were moved into a file called common.js because now two files use them. app-profile.ts
is one of them and the other is instance.ts
.
src/app-profile.ts
Outdated
@@ -29,7 +29,7 @@ export interface AppProfileOptions { | |||
* value is required when creating the app profile and optional when setting | |||
* the metadata. | |||
*/ | |||
routing?: 'any' | Cluster; | |||
routing?: 'any' | Cluster | Array<Cluster>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Java client uses a set instead of an array. Is there a compelling reason to have an array here? If no, can we switch to a set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good plan. I can change to a set. The only reason we would want to use an array is if we want for some reason for the order to be preserved every time we make a call to get the clusters in an app profile. I can't see why we would care about that though so a set is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only consideration is that in the response object we get an array so maybe we want an array input for consistency, but probably it doesn't matter.
Just an FYI, if you're working on a branch from a fork, add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after last small comment
test/instance.ts
Outdated
@@ -263,6 +256,23 @@ describe('Bigtable/Instance', () => { | |||
}; | |||
instance.createAppProfile(APP_PROFILE_ID, options, assert.ifError); | |||
}); | |||
|
|||
it('a set of clusters', done => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - update this description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this to say a set of cluster objects
, but I think you might have wanted something else. In this describe block our description string says, "should respect the routing option with"
and then each it
describes what we pass into the routing option. In this case, on the line that says, const options = {routing: new Set(clusters)};
we are setting the routing option to a set of clusters.
…into multi-cluster-routing
…ce/nodejs-bigtable into multi-cluster-routing
…into multi-cluster-routing # Conflicts: # package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, would appreciate if @bcoe could also sign off before merging :) thanks!
src/app-profile.ts
Outdated
) { | ||
// Runs if routing is a set and every element in it is a cluster | ||
appProfile.multiClusterRoutingUseAny = { | ||
clusterIds: [...options.routing].map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a great place to use an is
helper:
https://www.typescriptlang.org/docs/handbook/advanced-types.html#user-defined-type-guards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple non-blocking recommendations.
export const PREFIX = 'gcloud-tests-'; | ||
|
||
export function generateId(resourceType: string) { | ||
return PREFIX + resourceType + '-' + uuid.v1().substr(0, 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a timestamp to resource names, so that we can easily cleanup stale resources in the future:
Date.now()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a separate issue for this since I tried it and there was more cleanup for the sake of getting this PR in and keeping things separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts commit 7b862ef.
…into multi-cluster-routing
This PR allows users of the client library to provide a list of clusters in an API request so that routing of requests will stay restricted to that group of clusters.