feat: Multi cluster routing by danieljbruce · Pull Request #1007 · googleapis/nodejs-bigtable

danieljbruce · 2022-02-01T21:20:03Z

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.

…into multi-cluster-routing # Conflicts: # test/instance.ts

kolea2 · 2022-02-04T16:54:28Z

system-test/app-profile.ts

+        .slice(1, 3)
+        .map(clusterId => instance.cluster(clusterId));
+      const options = {
+        routing: clusterSubset,


I think it's fine (and a little easier to read) if you just hardcode 2/3 of the cluster ids

Sounds good. I'll go make this change.

kolea2 · 2022-02-04T16:57:32Z

system-test/app-profile.ts

+import {Bigtable} from '../src';
+import assert = require('assert');
+
+describe('📦 App Profile', () => {


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

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.

kolea2 · 2022-02-04T17:00:21Z

system-test/common.ts

+import {Cluster} from '../src/cluster';
+import * as inst from '../src/instance';
+
+export const PREFIX = 'gcloud-tests-';


is this cleanup from somewhere else? Should it be removed there?

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.

kolea2 · 2022-02-04T17:00:41Z

system-test/common.ts

+
+export const PREFIX = 'gcloud-tests-';
+
+export function generateId(resourceType: string) {


same question here

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.

kolea2 · 2022-02-04T17:02:16Z

src/app-profile.ts

   * the metadata.
   */
-  routing?: 'any' | Cluster;
+  routing?: 'any' | Cluster | Array<Cluster>;


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?

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.

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.

bcoe · 2022-03-16T22:02:32Z

Just an FYI, if you're working on a branch from a fork, add owlbot:run and kokoro:force-run to kick off tests/post-processing.

kolea2

lgtm after last small comment

kolea2 · 2022-04-13T18:46:57Z

test/instance.ts

        instance.createAppProfile(APP_PROFILE_ID, options, assert.ifError);
      });
+
+      it('a set of clusters', done => {


nit - update this description

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

kolea2

lgtm, would appreciate if @bcoe could also sign off before merging :) thanks!

bcoe · 2022-04-29T15:38:54Z

src/app-profile.ts

+      ) {
+        // Runs if routing is a set and every element in it is a cluster
+        appProfile.multiClusterRoutingUseAny = {
+          clusterIds: [...options.routing].map(


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

bcoe

Left a couple non-blocking recommendations.

bcoe · 2022-04-29T15:47:05Z

system-test/common.ts

+export const PREFIX = 'gcloud-tests-';
+
+export function generateId(resourceType: string) {
+  return PREFIX + resourceType + '-' + uuid.v1().substr(0, 8);


I would add a timestamp to resource names, so that we can easily cleanup stale resources in the future:

Date.now()

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.

This reverts commit 7b862ef.

…into multi-cluster-routing

danieljbruce added 4 commits January 31, 2022 17:48

Add third option in app profile config.

e587afc

First test

5886640

Fix a test case that broke

eb80c93

test for error when list is not clusters

065a904

danieljbruce requested a review from a team February 1, 2022 21:20

danieljbruce requested a review from a team as a code owner February 1, 2022 21:20

product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Feb 1, 2022

danieljbruce changed the title ~~Multi cluster routing~~ feat: Multi cluster routing Feb 1, 2022

danieljbruce added 3 commits February 1, 2022 16:42

Fix linter errors

0f5a073

Add a create app profile for an array of clusters

a507e2b

linting fixes

b3cda3f

kolea2 changed the base branch from multi-cluster-routing to main February 3, 2022 21:30

danieljbruce added 6 commits February 4, 2022 10:51

Working system test

157d33d

Remove unused imports

949d99a

linting fix

82c8628

Add to license

0de9665

Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …

91b9f33

…into multi-cluster-routing # Conflicts: # test/instance.ts

Update copyright year

5bf9127

kolea2 suggested changes Feb 4, 2022

View reviewed changes

danieljbruce added 11 commits February 4, 2022 17:08

PR fix 1

7d2eeea

linter fix

9cad4b7

Use a set instead of an array

d2105ae

Refactor that will make test writing easier

3a6324e

Second refactor

3b2122a

Another refactor to shorten the test

0bab061

before function is successful

991a796

create instance with clusters

c1e9574

Make individual test as short as possible

bfdaa99

single and multiple cluster tests

2a0931e

Added a third test

2bde408

bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 16, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 16, 2022

bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 16, 2022

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 22, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 22, 2022

kolea2 reviewed Apr 13, 2022

View reviewed changes

danieljbruce added 3 commits April 14, 2022 10:51

Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …

9d87cb9

…into multi-cluster-routing

Merge branch 'multi-cluster-routing' of https://github.com/danieljbru…

8c5b055

…ce/nodejs-bigtable into multi-cluster-routing

update a description

b66a126

product-auto-label bot added the size: l Pull request size is large. label Apr 14, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2022

danieljbruce added 2 commits April 19, 2022 17:27

grpc-gpc upgrade

b14181e

Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …

6f8c0f6

…into multi-cluster-routing # Conflicts: # package.json

kolea2 approved these changes Apr 22, 2022

View reviewed changes

bcoe reviewed Apr 29, 2022

View reviewed changes

bcoe approved these changes Apr 29, 2022

View reviewed changes

danieljbruce added 5 commits April 29, 2022 13:33

Use the refactored generateId

c5c16a6

fake cluster and types update

0d08e7d

Changed generate id code fragment

7b862ef

Revert "Changed generate id code fragment"

fd1d85b

This reverts commit 7b862ef.

Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …

cf98f9b

…into multi-cluster-routing

danieljbruce mentioned this pull request May 6, 2022

For the generateId function, add a date to generateId so that cleanup is easier #1084

Closed

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2022

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 10, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 10, 2022

danieljbruce merged commit 3468cd9 into googleapis:main May 10, 2022

release-please bot mentioned this pull request May 10, 2022

chore(main): release 3.13.0 #1075

Merged

release-please bot mentioned this pull request May 20, 2022

chore(main): release 4.0.0 #1091

Merged


		export const PREFIX = 'gcloud-tests-';

		export function generateId(resourceType: string) {

Conversation

danieljbruce commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe commented Mar 16, 2022

Uh oh!

kolea2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolea2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants