Address comments in the PR and add integration tests.

googleapis · Oct 12, 2023 · 228b7e5 · 228b7e5
1 parent 077e118
commit 228b7e5
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 77 deletions.
diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceAdminClientImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceAdminClientImpl.java
@@ -193,18 +193,6 @@ public Operation fromProto(Operation proto) {
   @Override
   public OperationFuture<Instance, CreateInstanceMetadata> createInstance(InstanceInfo instance)
       throws SpannerException {
-    boolean valid_capacity_with_autoscaling =
-        instance.getAutoscalingConfig() != null
-            && instance.getNodeCount() == 0
-            && instance.getProcessingUnits() == 0;
-    boolean valid_capacity_without_autoscaling =
-        (instance.getAutoscalingConfig() == null)
-            && (instance.getNodeCount() == 0 || instance.getProcessingUnits() == 0);
-
-    Preconditions.checkArgument(
-        valid_capacity_with_autoscaling || valid_capacity_without_autoscaling,
-        "Only one of nodeCount, processingUnits or autoscalingConfig can be set when creating a new instance");
-
     String projectName = PROJECT_NAME_TEMPLATE.instantiate("project", projectId);
     OperationFuture<com.google.spanner.admin.instance.v1.Instance, CreateInstanceMetadata>
         rawOperationFuture =

diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java
@@ -112,9 +112,7 @@ public Builder setProcessingUnits(int processingUnits) {
      * instance. Exactly one of processing units, node count, or autoscaling config must be set when
      * creating a new instance.
      */
-    public Builder setAutoscalingConfig(AutoscalingConfig autoscalingConfig) {
-      throw new UnsupportedOperationException("Unimplemented");
-    }
+    public abstract Builder setAutoscalingConfig(AutoscalingConfig autoscalingConfig);
 
     public abstract Builder setState(State state);
 

diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/InstanceAdminClientImplTest.java
@@ -19,7 +19,6 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.initMocks;
@@ -314,24 +313,6 @@ public void testCreateInstanceWithProcessingUnits() throws Exception {
     assertEquals(INSTANCE_NAME, operation.get().getId().getName());
   }
 
-  @Test
-  public void testCreateInstanceWithBothNodeCountAndProcessingUnits() throws Exception {
-    try {
-      client.createInstance(
-          InstanceInfo.newBuilder(InstanceId.of(PROJECT_ID, INSTANCE_ID))
-              .setInstanceConfigId(InstanceConfigId.of(PROJECT_ID, CONFIG_ID))
-              .setNodeCount(1)
-              .setProcessingUnits(100)
-              .build());
-      fail("missing expected exception");
-    } catch (IllegalArgumentException e) {
-      assertTrue(
-          e.getMessage()
-              .contains(
-                  "Only one of nodeCount, processingUnits or autoscalingConfig can be set when creating a new instance"));
-    }
-  }
-
   @Test
   public void testCreateInstanceWithAutoscalingConfig() throws Exception {
     OperationFuture<com.google.spanner.admin.instance.v1.Instance, CreateInstanceMetadata>
@@ -352,42 +333,6 @@ public void testCreateInstanceWithAutoscalingConfig() throws Exception {
     assertEquals(INSTANCE_NAME, operation.get().getId().getName());
   }
 
-  @Test
-  public void testCreateInstanceWithBothNodeCountAndAutoscalingConfig() throws Exception {
-    try {
-      client.createInstance(
-          InstanceInfo.newBuilder(InstanceId.of(PROJECT_ID, INSTANCE_ID))
-              .setInstanceConfigId(InstanceConfigId.of(PROJECT_ID, CONFIG_ID))
-              .setNodeCount(1)
-              .setAutoscalingConfig(getAutoscalingConfigProto())
-              .build());
-      fail("missing expected exception");
-    } catch (IllegalArgumentException e) {
-      assertTrue(
-          e.getMessage()
-              .contains(
-                  "Only one of nodeCount, processingUnits or autoscalingConfig can be set when creating a new instance"));
-    }
-  }
-
-  @Test
-  public void testCreateInstanceWithBothProcessingUnitsAndAutoscalingConfig() throws Exception {
-    try {
-      client.createInstance(
-          InstanceInfo.newBuilder(InstanceId.of(PROJECT_ID, INSTANCE_ID))
-              .setInstanceConfigId(InstanceConfigId.of(PROJECT_ID, CONFIG_ID))
-              .setProcessingUnits(1000)
-              .setAutoscalingConfig(getAutoscalingConfigProto())
-              .build());
-      fail("missing expected exception");
-    } catch (IllegalArgumentException e) {
-      assertTrue(
-          e.getMessage()
-              .contains(
-                  "Only one of nodeCount, processingUnits or autoscalingConfig can be set when creating a new instance"));
-    }
-  }
-
   @Test
   public void testGetInstance() {
     when(rpc.getInstance(INSTANCE_NAME)).thenReturn(getInstanceProto());

diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITInstanceAdminTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITInstanceAdminTest.java
@@ -21,14 +21,9 @@
 import static org.junit.Assume.assumeFalse;
 
 import com.google.api.gax.longrunning.OperationFuture;
-import com.google.cloud.spanner.Instance;
-import com.google.cloud.spanner.InstanceAdminClient;
-import com.google.cloud.spanner.InstanceConfig;
-import com.google.cloud.spanner.InstanceInfo;
-import com.google.cloud.spanner.IntegrationTestEnv;
-import com.google.cloud.spanner.Options;
-import com.google.cloud.spanner.ParallelIntegrationTest;
+import com.google.cloud.spanner.*;
 import com.google.common.collect.Iterators;
+import com.google.spanner.admin.instance.v1.AutoscalingConfig;
 import com.google.spanner.admin.instance.v1.UpdateInstanceMetadata;
 import java.util.ArrayList;
 import java.util.List;
@@ -121,6 +116,51 @@ public void updateInstance() throws Exception {
     instanceClient.updateInstance(toUpdate, InstanceInfo.InstanceField.DISPLAY_NAME).get();
   }
 
+  @Test
+  public void updateInstanceWithAutoscalingConfig() throws Exception {
+    assumeFalse(
+        "The emulator does not support updating instances with autoscaler", isUsingEmulator());
+
+    Instance instance =
+        instanceClient.getInstance(env.getTestHelper().getInstanceId().getInstance());
+    AutoscalingConfig autoscalingConfig =
+        AutoscalingConfig.newBuilder()
+            .setAutoscalingLimits(
+                AutoscalingConfig.AutoscalingLimits.newBuilder()
+                    .setMinProcessingUnits(1000)
+                    .setMaxProcessingUnits(2000))
+            .setAutoscalingTargets(
+                AutoscalingConfig.AutoscalingTargets.newBuilder()
+                    .setHighPriorityCpuUtilizationPercent(65)
+                    .setStorageUtilizationPercent(95))
+            .build();
+    InstanceInfo toUpdate =
+        InstanceInfo.newBuilder(env.getTestHelper().getInstanceId())
+            .setNodeCount(0)
+            .setAutoscalingConfig(autoscalingConfig)
+            .build();
+    OperationFuture<Instance, UpdateInstanceMetadata> op =
+        instanceClient.updateInstance(toUpdate, InstanceInfo.InstanceField.AUTOSCALING_CONFIG);
+    Instance newInstance = op.get();
+    assertThat(newInstance.getAutoscalingConfig()).isEqualTo(autoscalingConfig);
+
+    Instance newInstanceFromGet =
+        instanceClient.getInstance(env.getTestHelper().getInstanceId().getInstance());
+    assertThat(newInstanceFromGet).isEqualTo(newInstance);
+
+    toUpdate =
+        InstanceInfo.newBuilder(instance.getId())
+            .setAutoscalingConfig(null)
+            .setNodeCount(instance.getNodeCount())
+            .build();
+    instanceClient
+        .updateInstance(
+            toUpdate,
+            InstanceInfo.InstanceField.AUTOSCALING_CONFIG,
+            InstanceInfo.InstanceField.NODE_COUNT)
+        .get();
+  }
+
   @Test
   public void updateInstanceViaEntity() throws Exception {
     assumeFalse("The emulator does not support updating instances", isUsingEmulator());