feat: Add support for Managed Autoscaler #2624

claire921 · 2023-09-14T00:11:41Z

Add the ability to create / update an instance with autoscaling config
- Note that now user can only specify one of the node_count, processing_units or autoscaling_config as the compute capacity for an instance.
Add the ability to see autoscaling config with an instance

generated-files-bot · 2023-09-14T00:11:53Z

Warning: This pull request is touching the following templated files:

google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/instance/v1/InstanceAdminClientTest.java
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/Instance.java
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/InstanceOrBuilder.java
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/SpannerInstanceAdminProto.java
proto-google-cloud-spanner-admin-instance-v1/src/main/proto/google/spanner/admin/instance/v1/spanner_instance_admin.proto

google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java

arpan14 · 2023-10-09T12:06:55Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java

+      if (info.getAutoscalingConfig() != null) {
+        return new InstanceField[] {DISPLAY_NAME, AUTOSCALING_CONFIG, LABELS};
+      } else if (info.getNodeCount() > 0) {
+        return new InstanceField[] {DISPLAY_NAME, AUTOSCALING_CONFIG, NODE_COUNT, LABELS};


Why does this array contain both NODE_COUNT and AUTOSCALING_CONFIG ? It should contain just NODE_COUNT ? Could you check if any unit tests broke here or this was a silent error?

When you switch from an autoscaler instance to a non-autoscaler instance, the backend requires both the autoscaling_config and node_count field masks to make sure the intention is to clear the autoscaling_config and set a node_count.

LMK if there is a better alternative for this approach; otherwise I can add a comment for this to make it less confusing.

There are a few edge cases and open question here:

when getAutoscalingConfig != null, we are updating just AUTOSCALING_CONFIG. Shouldn't we have checks to include NODE_COUNT and PROCESSING_UNITS (to be set to 0) ?

If getNodeCount > 0, we are updating just NODE_COUNT. What if an instance already had PROCESSING_UNITS ? I assume the backend will throw a validation error. Similar question for the else condition.

If we are not marking PROCESSING_UNITS or NODE_COUNT as 0 in the second check, then why are we explicitly marking AUTOSCALING_CONFIG as null?

For #1, customers can only autoscaling_config once autoscaler is enabled. node_count and processing_units are output_only params. An instance can still have node_count and processing_units, it is just customers cannot modify them. This is consistent with CBT autoscaling API: https://screenshot.googleplex.com/6mi2r2QUKMfTk7x

For #2, this seems to be the logic before the autoscaler related changes. My understanding is that this method is to provide a default field mask when user did not specify one. I'm not sure why we did this in the first place -- as you pointed out, if an instance is specified with both nodes and processing units, this can be wrong. I'm following an existing pattern here.

For #3, if the autoscaling_config is not null, that means this is an autoscaler instance, and users should not specify processing_units or node_count as part of the field mask. If it is null, then the user intention will be to update a non-autoscaling instance.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java

arpan14 · 2023-10-09T12:09:39Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceAdminClientImpl.java

@@ -193,9 +193,18 @@ public Operation fromProto(Operation proto) {
  @Override
  public OperationFuture<Instance, CreateInstanceMetadata> createInstance(InstanceInfo instance)
      throws SpannerException {
+    boolean valid_capacity_with_autoscaling =
+        instance.getAutoscalingConfig() != null


Is this validation just specific to createInstance RPC? What about other RPCs ? I am wondering if these validations are better suited when we invoke setAutoscalingConfig method than within RPC.

WDYT?

Also, how are such validations supported in auto-generated languages (for ex - Golang) ?

Is this validation just specific to createInstance RPC? What about other RPCs ? I am wondering if these validations are better suited when we invoke setAutoscalingConfig method than within RPC.

When they updateInstance, they should also only specify one of the PU, nodes or autoscaling config.

The check was here before so I modified it to take autoscaling config into account, but I can move it to another place if that makes more sense.

Also, how are such validations supported in auto-generated languages (for ex - Golang) ?

So the backend will do the validation check any way, so if it is auto-generated languages, it will be rejected by the backend.

Sure, let's get rid of the validation all together and rely on the backend validation. Some simpler validation on primitive types can be considered to be moved to the protos. This in general is preferred since we are gradually moving in a direction on how we can simplify our handwritten clients (similar to our auto-generated clients). So, this would be a step in that direction.

snippet-bot · 2023-10-12T16:11:03Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

(cherry picked from commit 2d43eb8)

(cherry picked from commit 228b7e5)

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java

arpan14 · 2023-10-16T09:03:48Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITInstanceAdminTest.java

-import com.google.cloud.spanner.IntegrationTestEnv;
-import com.google.cloud.spanner.Options;
-import com.google.cloud.spanner.ParallelIntegrationTest;
+import com.google.cloud.spanner.*;


Revert this change and bring back the full imports. I think this could be an IDE setting which might be automatically opimizing the imports. You may need to switch off this IDE setting. Reference - https://www.jetbrains.com/help/idea/creating-and-optimizing-imports.html#optimize-imports

google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITInstanceAdminTest.java

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceTest.java

arpan14 · 2023-10-16T10:34:29Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java

+      if (info.getAutoscalingConfig() != null) {
+        return new InstanceField[] {DISPLAY_NAME, AUTOSCALING_CONFIG, LABELS};
+      } else if (info.getNodeCount() > 0) {
+        return new InstanceField[] {DISPLAY_NAME, AUTOSCALING_CONFIG, NODE_COUNT, LABELS};


There are a few edge cases and open question here:

when getAutoscalingConfig != null, we are updating just AUTOSCALING_CONFIG. Shouldn't we have checks to include NODE_COUNT and PROCESSING_UNITS (to be set to 0) ?

If getNodeCount > 0, we are updating just NODE_COUNT. What if an instance already had PROCESSING_UNITS ? I assume the backend will throw a validation error. Similar question for the else condition.

If we are not marking PROCESSING_UNITS or NODE_COUNT as 0 in the second check, then why are we explicitly marking AUTOSCALING_CONFIG as null?

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java

arpan14 · 2023-10-19T05:19:31Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java

                .build()))
        .thenReturn(rawOperationFuture);
    InstanceInfo instanceInfo =
        InstanceInfo.newBuilder(InstanceId.of(INSTANCE_NAME))
            .setInstanceConfigId(InstanceConfigId.of(CONFIG_NAME))
            .setNodeCount(3)
            .setProcessingUnits(3000)
+            .setAutoscalingConfig(getAutoscalingConfigProto())
            .build();
    OperationFuture<Instance, UpdateInstanceMetadata> operationWithFieldMask =
        client.updateInstance(instanceInfo);


Can we add more assertions on what we expect the node count, processing unit and autoscaling config to contain, similar to how we are asserting INSTANCE_NAME ?

Same above.

arpan14 · 2023-10-19T05:22:29Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java

+    OperationFuture<Instance, UpdateInstanceMetadata> operationWithFieldMask =
+        client.updateInstance(instanceInfo);
+    assertTrue(operationWithFieldMask.isDone());
+    assertEquals(INSTANCE_NAME, operationWithFieldMask.get().getId().getName());


Could you add more assertions on fields being updated?

arpan14 · 2023-10-19T05:38:41Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java

+    OperationFuture<Instance, UpdateInstanceMetadata> operation =
+        client.updateInstance(instanceInfo);
+    assertTrue(operation.isDone());
+    assertEquals(INSTANCE_NAME, operation.get().getId().getName());


Can we add few more assertions on value of nodecount, processing units and autoscaling config. Applicable at other tests as well.

The assertions are probably not needed, because we mock the RPC to return an instance only with processing_units: https://screenshot.googleplex.com/7fF5jTJZ8hbQ2nn. So if we add assertions, the assertions will only be to test the mocking of the RPC, instead of the actual code.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

🤖 I have created a release *beep* *boop* --- ## [6.52.0](https://togithub.com/googleapis/java-spanner/compare/v6.51.0...v6.52.0) (2023-10-19) ### Features * Add support for Managed Autoscaler ([#2624](https://togithub.com/googleapis/java-spanner/issues/2624)) ([e5e6923](https://togithub.com/googleapis/java-spanner/commit/e5e6923a351670ab237c411bb4a549533dac1b6b)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

claire921 requested review from a team as code owners September 14, 2023 00:11

product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Sep 14, 2023

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 19, 2023

claire921 changed the title ~~feat: add support for Managed Autoscaler~~ feat: Add support for Managed Autoscaler Sep 19, 2023

arpan14 reviewed Oct 9, 2023

View reviewed changes

arpan14 closed this Oct 13, 2023

arpan14 force-pushed the autoscaler branch from 228b7e5 to 8d38ca3 Compare October 13, 2023 09:42

product-auto-label bot added size: u Pull request is empty. and removed size: xl Pull request size is extra large. labels Oct 13, 2023

claire921 added 2 commits October 13, 2023 10:47

add the handwritten layer support for autoscaler

c36a99a

(cherry picked from commit 2d43eb8)

Address comments in the PR and add integration tests.

036601a

(cherry picked from commit 228b7e5)

arpan14 reopened this Oct 13, 2023

product-auto-label bot added size: l Pull request size is large. and removed size: u Pull request is empty. labels Oct 13, 2023

retrigger checks

3930403

arpan14 reviewed Oct 16, 2023

View reviewed changes

claire921 added 2 commits October 16, 2023 13:16

Address new comments

bbfe85a

fix the abstract method issue

d9c1acc

arpan14 reviewed Oct 18, 2023

View reviewed changes

Fix the instanceadminclientimpltest

e97f68c

arpan14 approved these changes Oct 19, 2023

View reviewed changes

arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 19, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 19, 2023

🦉 Updates from OwlBot post-processor

40287ea

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

arpan14 added the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2023

arpan14 added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 19, 2023

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 19, 2023

gcf-merge-on-green bot merged commit e5e6923 into googleapis:main Oct 19, 2023
22 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2023

release-please bot mentioned this pull request Oct 19, 2023

chore(main): release 6.52.0 #2688

Merged

surbhigarg92 mentioned this pull request Feb 22, 2024

feat: Managed Autoscaler googleapis/nodejs-spanner#1965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Managed Autoscaler #2624

feat: Add support for Managed Autoscaler #2624

claire921 commented Sep 14, 2023 •

edited by arpan14

generated-files-bot bot commented Sep 14, 2023 •

edited

arpan14 Oct 9, 2023

claire921 Oct 10, 2023

arpan14 Oct 16, 2023

claire921 Oct 16, 2023

arpan14 Oct 9, 2023

arpan14 Oct 9, 2023

claire921 Oct 10, 2023

arpan14 Oct 11, 2023

snippet-bot bot commented Oct 12, 2023 •

edited

arpan14 Oct 16, 2023

claire921 Oct 16, 2023

arpan14 Oct 16, 2023

arpan14 Oct 19, 2023

claire921 Oct 19, 2023

arpan14 Oct 19, 2023

claire921 Oct 19, 2023

arpan14 Oct 19, 2023

claire921 Oct 19, 2023

feat: Add support for Managed Autoscaler #2624

feat: Add support for Managed Autoscaler #2624

Conversation

claire921 commented Sep 14, 2023 • edited by arpan14

generated-files-bot bot commented Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Oct 12, 2023 • edited

No region tags are edited in this PR.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claire921 commented Sep 14, 2023 •

edited by arpan14

generated-files-bot bot commented Sep 14, 2023 •

edited

snippet-bot bot commented Oct 12, 2023 •

edited