Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for Managed Autoscaler #2624

Merged
merged 7 commits into from Oct 19, 2023
Merged

feat: Add support for Managed Autoscaler #2624

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c36a99a
add the handwritten layer support for autoscaler
claire921 Sep 1, 2023
036601a
Address comments in the PR and add integration tests.
claire921 Oct 12, 2023
3930403
retrigger checks
claire921 Oct 13, 2023
bbfe85a
Address new comments
claire921 Oct 16, 2023
d9c1acc
fix the abstract method issue
claire921 Oct 17, 2023
e97f68c
Fix the instanceadminclientimpltest
claire921 Oct 18, 2023
40287ea
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/Instance.java
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
import com.google.cloud.spanner.Options.ListOption;
import com.google.longrunning.Operation;
import com.google.spanner.admin.database.v1.CreateDatabaseMetadata;
import com.google.spanner.admin.instance.v1.AutoscalingConfig;
import com.google.spanner.admin.instance.v1.UpdateInstanceMetadata;
import java.util.Map;

Expand Down Expand Up @@ -86,6 +87,12 @@ public Builder setProcessingUnits(int processingUnits) {
return this;
}

@Override
public Builder setAutoscalingConfig(AutoscalingConfig autoscalingConfig) {
infoBuilder.setAutoscalingConfig(autoscalingConfig);
return this;
}

@Override
public Builder setState(State state) {
infoBuilder.setState(state);
Expand Down Expand Up @@ -220,6 +227,7 @@ static Instance fromProto(
.setNodeCount(proto.getNodeCount())
.setCreateTime(Timestamp.fromProto(proto.getCreateTime()))
.setUpdateTime(Timestamp.fromProto(proto.getUpdateTime()))
.setAutoscalingConfig(proto.getAutoscalingConfig())
.setProcessingUnits(proto.getProcessingUnits());
State state;
switch (proto.getState()) {
Expand Down
3 changes: 0 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceAdminClientImpl.java
View file
Open in desktop
Expand Up @@ -193,9 +193,6 @@ public Operation fromProto(Operation proto) {
@Override
public OperationFuture<Instance, CreateInstanceMetadata> createInstance(InstanceInfo instance)
throws SpannerException {
Preconditions.checkArgument(
instance.getNodeCount() == 0 || instance.getProcessingUnits() == 0,
"Only one of nodeCount and processingUnits 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 =
Expand Down
53 changes: 44 additions & 9 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/InstanceInfo.java
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableMap;
import com.google.protobuf.FieldMask;
import com.google.spanner.admin.instance.v1.AutoscalingConfig;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
Expand All @@ -35,13 +36,16 @@ public enum InstanceField implements FieldSelector {
DISPLAY_NAME("display_name"),
NODE_COUNT("node_count"),
PROCESSING_UNITS("processing_units"),
AUTOSCALING_CONFIG("autoscaling_config"),
LABELS("labels");

static InstanceField[] defaultFieldsToUpdate(InstanceInfo info) {
if (info.getNodeCount() > 0) {
return new InstanceField[] {DISPLAY_NAME, NODE_COUNT, LABELS};
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};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few edge cases and open question here:

  1. 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) ?
  2. 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.
  3. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
return new InstanceField[] {DISPLAY_NAME, PROCESSING_UNITS, LABELS};
return new InstanceField[] {DISPLAY_NAME, AUTOSCALING_CONFIG, PROCESSING_UNITS, LABELS};
arpan14 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -87,21 +91,31 @@ Builder setCreateTime(Timestamp createTime) {
}

/**
* Sets the number of nodes for the instance. Exactly one of processing units or node count must
* be set when creating a new instance.
* Sets the number of nodes for the instance. Exactly one of processing units, node count or
* autoscaling config must be set when creating a new instance.
arpan14 marked this conversation as resolved.
Show resolved Hide resolved
*/
public abstract Builder setNodeCount(int nodeCount);

/**
* Sets the number of processing units for the instance. Exactly one of processing units or node
* count must be set when creating a new instance. Processing units must be between 1 and 999
* (inclusive) when creating a new instance with node count = 0. Processing units from 1000 and
* up must always be a multiple of 1000 (that is equal to an integer number of nodes).
* Sets the number of processing units for the instance. Exactly one of processing units, node
* count, or autoscaling config must be set when creating a new instance. Processing units must
* be between 1 and 999 (inclusive) when creating a new instance with node count = 0. Processing
* units from 1000 and up must always be a multiple of 1000 (that is equal to an integer number
* of nodes).
*/
public Builder setProcessingUnits(int processingUnits) {
throw new UnsupportedOperationException("Unimplemented");
}

/**
* Sets the autoscaling config for the instance, which will enable the autoscaling for this
* instance. Exactly one of processing units, node count, or autoscaling config must be set when
* creating a new instance.
*/
public Builder setAutoscalingConfig(AutoscalingConfig autoscalingConfig) {
arpan14 marked this conversation as resolved.
Show resolved Hide resolved
throw new UnsupportedOperationException("Unimplemented");
}

public abstract Builder setState(State state);

public abstract Builder addLabel(String key, String value);
Expand All @@ -117,6 +131,7 @@ static class BuilderImpl extends Builder {
private String displayName;
private int nodeCount;
private int processingUnits;
private AutoscalingConfig autoscalingConfig;
private State state;
private Map<String, String> labels;
private Timestamp updateTime;
Expand All @@ -133,6 +148,7 @@ static class BuilderImpl extends Builder {
this.displayName = instance.displayName;
this.nodeCount = instance.nodeCount;
this.processingUnits = instance.processingUnits;
this.autoscalingConfig = instance.autoscalingConfig;
this.state = instance.state;
this.labels = new HashMap<>(instance.labels);
this.updateTime = instance.updateTime;
Expand Down Expand Up @@ -175,6 +191,12 @@ public BuilderImpl setProcessingUnits(int processingUnits) {
return this;
}

@Override
public BuilderImpl setAutoscalingConfig(AutoscalingConfig autoscalingConfig) {
this.autoscalingConfig = autoscalingConfig;
return this;
}

@Override
public BuilderImpl setState(State state) {
this.state = state;
Expand Down Expand Up @@ -204,6 +226,7 @@ public InstanceInfo build() {
private final String displayName;
private final int nodeCount;
private final int processingUnits;
private final AutoscalingConfig autoscalingConfig;
private final State state;
private final ImmutableMap<String, String> labels;
private final Timestamp updateTime;
Expand All @@ -215,6 +238,7 @@ public InstanceInfo build() {
this.displayName = builder.displayName;
this.nodeCount = builder.nodeCount;
this.processingUnits = builder.processingUnits;
this.autoscalingConfig = builder.autoscalingConfig;
this.state = builder.state;
this.labels = ImmutableMap.copyOf(builder.labels);
this.updateTime = builder.updateTime;
Expand Down Expand Up @@ -254,6 +278,11 @@ public int getProcessingUnits() {
return processingUnits;
}

/** Returns the autoscaling config of the instance. */
public AutoscalingConfig getAutoscalingConfig() {
return autoscalingConfig;
}

/** Returns the current state of the instance. */
public State getState() {
return state;
Expand All @@ -276,6 +305,7 @@ public String toString() {
.add("displayName", displayName)
.add("nodeCount", nodeCount)
.add("processingUnits", processingUnits)
.add("autoscaling_config", autoscalingConfig)
.add("state", state)
.add("labels", labels)
.add("createTime", createTime)
Expand All @@ -297,6 +327,7 @@ public boolean equals(Object o) {
&& Objects.equals(displayName, that.displayName)
&& nodeCount == that.nodeCount
&& processingUnits == that.processingUnits
&& Objects.equals(autoscalingConfig, that.autoscalingConfig)
&& state == that.state
&& Objects.equals(labels, that.labels)
&& Objects.equals(updateTime, that.updateTime)
Expand All @@ -311,6 +342,7 @@ public int hashCode() {
displayName,
nodeCount,
processingUnits,
autoscalingConfig,
state,
labels,
updateTime,
Expand All @@ -330,6 +362,9 @@ com.google.spanner.admin.instance.v1.Instance toProto() {
if (getInstanceConfigId() != null) {
builder.setConfig(getInstanceConfigId().getName());
}
if (getAutoscalingConfig() != null) {
builder.setAutoscalingConfig(getAutoscalingConfig());
}
return builder.build();
}

Expand Down