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.
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
feat(bigtable): add support for autoscaling #5232
Conversation
googleapis/go-genproto#733 has been merged. Should now just need to update the version of genproto being pulled in. |
Thanks @codyoss! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple thoughts and questions, hopefully some of this is helpful!
This commit adds support for autoscaling in the InstanceAdminClient with associated unit tests and a happy-path integration test. When creating either a new instance or cluster, callers may specify an AutoscalingConfig. For existing clusters, to enable autoscaling on a cluster, a caller may use SetAutoscaling. To remove autoscaling, callers may use UpdateCluster. Alternatively, callers may either update the cluster to enable or disable autoscaling with UpdateInstanceWithClusters or UpdateInstanceAndSyncClusters.
Previously UpdateInstanceWithClusters would try to update a cluster even if the NumNodes was 0 (or less), resulting in an erroneous error. This commit ensures a cluster is updated only when a valid and non-zero value of NumNodes is provided.
aa01df7
to
49b09c9
Compare
@telpirion and @tritone PTAL. I've addressed all the comments above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple more minor comments/questions, overall LGTM. I will leave it to @kolea2 and @telpirion to approve!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly reviewing from an API perspective but looks really good! A few questions/comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit, otherwise LGTM
See googleapis/googleapis@0fd6a32 for details.
Note: this work depends on googleapis/googleapis@0fd6a32 (from Nov. 15) being available in go-genproto. (last updated Nov. 2).