feat: Autoscaler by danieljbruce · Pull Request #1077 · googleapis/nodejs-bigtable

danieljbruce · 2022-04-21T21:32:53Z

This feature allows the user to set properties when creating a cluster or updating a cluster that turn on the autoscaler or turn it off to use manual scaling.

…into autoscaler

kolea2 · 2022-04-27T14:24:21Z

src/cluster.ts

 ) => void;
 export interface SetClusterMetadataOptions {
-  nodes: number;
+  nodes?: number;


is this considered breaking if we change something from required to optional? I'm guessing it would be the other way around, but want to confirm :)

Here we are changing something from required to optional so that would make it not a breaking change since users can still pass just nodes in. This is where we make nodes optional because instead the user can provide autoscaling parameters. These parameters will be validated against a new validation function and an error will be thrown if an invalid combination of parameters are provided.

I would not consider switching from required to optional for TypeScript breaking 👍

src/cluster.ts

src/instance.ts

kolea2 · 2022-04-28T13:59:27Z

src/utils/cluster.ts

+    if (
+      metadata.cpuUtilizationPercent ||
+      metadata.minServeNodes ||
+      metadata.maxServeNodes


what happens if only one of these are set? Let's say metadata.minServeNodes = 4, but the rest are unset. Could that cause issues?

When we use this in instance.ts we validate beforehand so in this case an error would be thrown as required, but after investigating this further, I think I will add validation to createInstance as well to make sure an error gets thrown if properties are not set properly for each cluster.

When creating instances we now validate inputs.

kolea2 · 2022-04-28T14:01:06Z

system-test/cluster.ts

+            assert.equal(e.message, ClusterUtils.noConfigError);
+          }
+        });
+        it('By providing too much cluster configurations', async () => {


nit - reword to express that the user entered manual AND autoscaling configurations

system-test/cluster.ts

danieljbruce

Initial follow-up to first review.

danieljbruce · 2022-04-28T16:58:19Z

src/utils/cluster.ts

+    if (
+      metadata.cpuUtilizationPercent ||
+      metadata.minServeNodes ||
+      metadata.maxServeNodes


When we use this in instance.ts we validate beforehand so in this case an error would be thrown as required, but after investigating this further, I think I will add validation to createInstance as well to make sure an error gets thrown if properties are not set properly for each cluster.

system-test/cluster.ts

danieljbruce · 2022-04-28T17:13:30Z

system-test/cluster.ts

+            assert.equal(e.message, ClusterUtils.noConfigError);
+          }
+        });
+        it('By providing too much cluster configurations', async () => {


kolea2 · 2022-04-28T17:40:06Z

system-test/cluster.ts

+      it('Create an instance with clusters for manual scaling', async () => {
+        await checkMetadata(cluster, 2);
+      });
+      it('Create an instance and then create a cluster for manual scaling', async () => {


nit - here and in all other it(.. descriptions, I think they should be reworded slightly; for example, here I would change it to say:

it('should create an instance and then create a cluster with manual scaling')

Let me know your thoughts :)

Very good point. I'll look at some of the other PRs and make sure that they have these descriptions as well.

danieljbruce

Addressed two more outstanding comments

danieljbruce · 2022-04-28T20:12:41Z

system-test/cluster.ts

+      it('Create an instance with clusters for manual scaling', async () => {
+        await checkMetadata(cluster, 2);
+      });
+      it('Create an instance and then create a cluster for manual scaling', async () => {


Very good point. I'll look at some of the other PRs and make sure that they have these descriptions as well.

danieljbruce · 2022-04-28T20:13:19Z

src/utils/cluster.ts

+    if (
+      metadata.cpuUtilizationPercent ||
+      metadata.minServeNodes ||
+      metadata.maxServeNodes


When creating instances we now validate inputs.

kolea2 · 2022-05-04T17:02:34Z

src/utils/cluster.ts

+  static incompleteConfigError =
+    'All of autoscaling configurations must be specified at the same time (min_serve_nodes, max_serve_nodes, and cpu_utilization_percent).';
+
+  static validateMetadata(


I would rename this validateCreateClusterMetadata as we are checking for all autoscaling settings set.

How about validateClusterMetadata since sometimes this check is done when updating a cluster as well as creating it?

kolea2 · 2022-05-04T17:04:43Z

system-test/cluster.ts

+            assert.equal(e.message, ClusterUtils.allConfigError);
+          }
+        });
+        it('should throw an error providing all autoscaling configurations', async () => {


should this say "should throw an error when missing some autoscaling configurations"?

kolea2 · 2022-05-04T17:05:53Z

system-test/cluster.ts

+      });
+      it('should create an instance and then create clusters for automatic scaling', async () => {
+        const clusterId: string = generateId('cluster');
+        await createStandardNewInstance(clusterId, 2);


why are we creating a cluster with manual scaling for this test?

This test is designed to capture a particular edge case. The test before this already covers the case where we create an instance that has clusters with automatic scaling. In this case we want to make sure everything works if we create an instance and then create a cluster since that user experience uses a different function entirely.

This test case does the following:

createStandardNewInstance creates an instance with a cluster that has manual scaling

cluster.create(createClusterOptions) creates a cluster with automatic scaling on this instance

checkMetadata ensures that the metadata for the cluster created with automatic scaling has nodes, minServeNodes, maxServeNodes, cpuUtilizationPercent set correctly.

kolea2 · 2022-05-04T17:06:43Z

system-test/cluster.ts

+
+      it('should change nodes for manual scaling', async () => {
+        const updateNodes = 5;
+        assert.notEqual(startingNodes, updateNodes);


this check is comparing two int variables - if you want to have this, I would instead get the creater cluster's nodes, then compare the created cluster's nodes to the updateNodes value.

checkMetadata does the second thing you mentioned though it checks metadata.serveNodes equals updateNodes rather than checking metadata.serveNodes does not equal startingNodes. This assert.notEqual is more of a maintainability thing to ensure the programmer doesn't set updateNodes to equal startingNodes and then have the test pass when setMetadata doesn't actually work properly. If it's confusing though then we can remove it.

I don't think this is providing much in terms of autoscaling assertions, so I would remove it in favor of the checks you already mentioned.

kolea2 · 2022-05-04T17:07:43Z

system-test/cluster.ts

+    });
+  });
+  describe('Update cluster', () => {
+    describe('Starting from manual scaling', () => {


I would rename this description to something like "updating manual scaling for a cluster"

kolea2 · 2022-05-04T17:09:02Z

system-test/cluster.ts

+            assert.equal(e.message, ClusterUtils.allConfigError);
+          }
+        });
+        it('should throw an error providing all autoscaling configurations', async () => {


i think this is the wrong description for this test?

Yeah. This is a confusing error. Related to the comment above. Changing this now.

kolea2 · 2022-05-04T17:10:41Z

system-test/cluster.ts

+  async function checkMetadata(
+    cluster: Cluster,
+    compareValues: SetClusterMetadataOptions,
+    isConfigDefined: boolean


why are we passing this into this function? Shouldn't we be able to determine this?

This is meant to be the expected value rather than the actual value of an assertion check. The purpose of passing isConfigDefined in is just to make sure the test is more thorough. For example, if we have a test that creates a cluster with automatic scaling and then we find the metadata fetched has no clusterConfig defined then we want it to fail on the line of code that says:

assert.equal(isConfigDefined, false);

bcoe · 2022-05-27T19:16:46Z

src/cluster.ts

-        callback(err, resp);
-      }
-    );
+    const setMetadataWithLocation = (location: string, name: string) => {


I would remove this logic if we think it's currently not needed based on your testing.

bcoe · 2022-05-27T19:19:38Z

src/utils/cluster.ts

+      metadata
+    );
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    delete (cluster as any).nodes;


I don't think you should have to case to any, assuming that these are properties on ICluster.

…into autoscaler # Conflicts: # test/cluster.ts

danieljbruce added 19 commits April 12, 2022 16:40

work in progress - broken

c0630df

Add autoscaling options to interface

b1b898d

Work in progress on function for setting metadata

ea8860c

Fixed some tests that were broken due to the chang

d62bb01

Cluster.ts in test file

8f58902

Update cluster creation to accept new parameters

96c71eb

Fix for failing test

9eafb76

Enhance system tests for cluster

5ef31ef

Add another test

ce608de

condense tests

13097f5

Added more tests

3498c9f

Test improvements

bd780e5

Change update mask to get manual scaling right

4270a6a

Added validation for cluster creation configs

67f6645

Refactor cluster object in tests

370d41c

Another cluster id refactor

72d54ea

Remove TODO that no longer applies

152d6e3

Code reorganization for test cases

059aa4e

Fix the test so that it breaks for good reason

51ec882

danieljbruce requested a review from a team as a code owner April 21, 2022 21:32

danieljbruce requested a review from a team April 21, 2022 21:32

product-auto-label bot added size: l Pull request size is large. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Apr 21, 2022

danieljbruce marked this pull request as draft April 21, 2022 21:33

danieljbruce added 6 commits April 22, 2022 10:18

update mask push fix

bc32192

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

822bd6d

…into autoscaler

Add nodes in all calls to update and create cluste

4798915

Add license headers

19ac452

correct copyright year

931b25c

Add singlequote

9ea29bb

danieljbruce marked this pull request as ready for review April 22, 2022 17:43

kolea2 reviewed Apr 27, 2022

View reviewed changes

kolea2 suggested changes Apr 28, 2022

View reviewed changes

danieljbruce added 2 commits April 28, 2022 13:11

Add a validation

61eb533

Update test descriptions

b4ad269

danieljbruce commented Apr 28, 2022

View reviewed changes

kolea2 reviewed Apr 28, 2022

View reviewed changes

danieljbruce added 3 commits April 28, 2022 14:27

refactor to use one check metadata function

8ec8232

Fix tests as specifying nodes is required anyway

a95bf13

should added to test case descriptions

961697d

danieljbruce commented Apr 28, 2022

View reviewed changes

kolea2 suggested changes May 4, 2022

View reviewed changes

PR updates

749ac86

bcoe self-assigned this May 18, 2022

bcoe suggested changes May 27, 2022

View reviewed changes

danieljbruce added 4 commits May 27, 2022 16:05

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

d759f43

…into autoscaler # Conflicts: # test/cluster.ts

validation error

c7cf8a2

Remove call to get metadata

b4025ed

PR updates

232cc0d

bcoe approved these changes May 30, 2022

View reviewed changes

kolea2 approved these changes May 31, 2022

View reviewed changes

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

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

danieljbruce merged commit e5f6fdb into googleapis:main May 31, 2022

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

chore(main): release 4.0.0 #1091

Merged

Conversation

danieljbruce commented Apr 21, 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!

Uh oh!

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!

Uh oh!

danieljbruce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

danieljbruce 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!

danieljbruce May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

danieljbruce May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

danieljbruce May 10, 2022 •

edited

Loading

danieljbruce May 10, 2022 •

edited

Loading