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.

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

Merged
merged 11 commits into from Jan 19, 2022
Merged

feat(bigtable): add support for autoscaling #5232

merged 11 commits into from Jan 19, 2022

Conversation

enocom
Copy link
Member

@enocom enocom commented Dec 21, 2021

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).

@enocom enocom requested review from crwilcox, dmahugh, tritone and a team as code owners December 21, 2021 16:42
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Dec 21, 2021
@codyoss
Copy link
Member

codyoss commented Dec 21, 2021

googleapis/go-genproto#733 has been merged. Should now just need to update the version of genproto being pulled in.

@enocom
Copy link
Member Author

enocom commented Dec 21, 2021

Thanks @codyoss!

Copy link
Contributor

@tritone tritone left a 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!

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
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.
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jan 11, 2022
@enocom enocom requested a review from tritone January 11, 2022 21:58
@enocom
Copy link
Member Author

enocom commented Jan 11, 2022

@telpirion and @tritone PTAL. I've addressed all the comments above.

Copy link
Contributor

@tritone tritone left a 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!

bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/admin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kolea2 kolea2 left a 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

bigtable/admin.go Outdated Show resolved Hide resolved
bigtable/admin.go Show resolved Hide resolved
bigtable/integration_test.go Outdated Show resolved Hide resolved
@enocom enocom requested a review from kolea2 January 18, 2022 22:39
Copy link
Contributor

@tritone tritone left a 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

bigtable/admin.go Show resolved Hide resolved
@enocom enocom merged commit a59d1ac into main Jan 19, 2022
@enocom enocom deleted the bigtable-autoscaling branch January 19, 2022 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants