feat(spanner): add spanner sample snippet for creating asymmetric autoscaling instance #4845
Conversation
product-auto-label
bot
added
the
samples
![code-review-assist[bot]](https://avatars.githubusercontent.com/in/1008254?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @hibiken and reviewers,
I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:
This pull request adds a new Spanner sample snippet demonstrating how to create a Cloud Spanner instance with asymmetric autoscaling. The intent is to provide a clear example of configuring asymmetric autoscaling, allowing for independent scaling of read-only replicas across different regions. The changes include:
- Addition of a new Go file:
spanner/spanner_snippets/spanner/spanner_create_instance_with_asymmetric_autoscaling.goThis file contains the code for creating a Spanner instance with asymmetric autoscaling configuration. The code creates an instance with a specified configuration, display name, autoscaling limits and targets, and importantly, asymmetric autoscaling options for read-only replicas in specific regions (europe-west1, europe-west4, and asia-east1). - Updates to
go.modandgo.sum: These files are updated to reflect the addition of new dependencies required by the new sample snippet. Specifically, several indirect dependencies are added or updated to their latest versions. - Update to
spanner/spanner_snippets/spanner/integration_test.go: The integration test file is updated to include a test case for the newcreateInstanceWithAsymmetricAutoscalingConfigfunction.
The overall effect is to expand the existing Spanner sample collection with a practical example of a more advanced configuration option.
And now, a little haiku to lighten the mood:
Cloud Spanner scales,
Regions rise and fall in sync,
Data flows so free.
hibiken
changed the title
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
There was a problem hiding this comment.
This pull request adds a new sample demonstrating how to create a Spanner instance with an asymmetric autoscaling configuration. The code is well-structured and easy to understand. The addition of this sample enhances the documentation and provides a valuable example for users interested in asymmetric autoscaling. The changes in go.mod and go.sum are appropriate and reflect the dependencies required for the new sample. The integration test covers the new sample function, ensuring its correctness. Overall, the pull request is well-done and ready for approval.
spanner/spanner_snippets/spanner/spanner_create_instance_with_asymmetric_autoscaling.go
Outdated
Show resolved
Hide resolved
| @@ -129,6 +129,7 @@ func TestCreateInstances(t *testing.T) { | |||
| runCreateAndUpdateInstanceSample(t, createInstanceWithoutDefaultBackupSchedule, updateInstanceDefaultBackupScheduleType) | |||
| runCreateInstanceSample(t, createInstanceWithProcessingUnits) | |||
| runCreateInstanceSample(t, createInstanceWithAutoscalingConfig) | |||
| runCreateInstanceSample(t, createInstanceWithAsymmetricAutoscalingConfig) | |||
There was a problem hiding this comment.
It's a good practice to add a test case for the new sample function createInstanceWithAsymmetricAutoscalingConfig to ensure its correctness. Consider adding a test case similar to the existing ones, which checks the output of the function and verifies the instance creation.
| runCreateInstanceSample(t, createInstanceWithAsymmetricAutoscalingConfig) | |
| runCreateInstanceSample(t, createInstanceWithAsymmetricAutoscalingConfig) |
There was a problem hiding this comment.
nit: I would prefer to break out new tests into separate test cases whenever possible. Inline test executions like this are hard to debug and can cause merge conflicts when multiple authors are working on the same file. This change is not required, but strongly suggested.
See: https://googlecloudplatform.github.io/samples-style-guide/#dedicated-testing-per-sample
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
…asymmetric_autoscaling.go Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com>
| @@ -129,6 +129,7 @@ func TestCreateInstances(t *testing.T) { | |||
| runCreateAndUpdateInstanceSample(t, createInstanceWithoutDefaultBackupSchedule, updateInstanceDefaultBackupScheduleType) | |||
| runCreateInstanceSample(t, createInstanceWithProcessingUnits) | |||
| runCreateInstanceSample(t, createInstanceWithAutoscalingConfig) | |||
| runCreateInstanceSample(t, createInstanceWithAsymmetricAutoscalingConfig) | |||
There was a problem hiding this comment.
nit: I would prefer to break out new tests into separate test cases whenever possible. Inline test executions like this are hard to debug and can cause merge conflicts when multiple authors are working on the same file. This change is not required, but strongly suggested.
See: https://googlecloudplatform.github.io/samples-style-guide/#dedicated-testing-per-sample
| "google.golang.org/genproto/protobuf/field_mask" | ||
| ) | ||
|
|
||
| // Example of creating an asymmetric autoscaling enabled instance in Go. |
There was a problem hiding this comment.
nit: use typical Godoc styling where the first word of the comment is the same as the name of the function.
| if err != nil { | ||
| return fmt.Errorf("waiting for instance creation to finish failed: %w", err) | ||
| } | ||
| // The instance may not be ready to serve yet. |
There was a problem hiding this comment.
I like the helpful comments in this sample!
spanner/spanner_snippets/spanner/spanner_create_instance_with_asymmetric_autoscaling.go
Outdated
Show resolved
Hide resolved
Description
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)