chore(spanner): Add support for experimental endpoint#5527
chore(spanner): Add support for experimental endpoint#5527mayurkale22 wants to merge 2 commits intoGoogleCloudPlatform:mainfrom
Conversation
product-auto-label
Bot
added
api: spanner
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for an experimental host in Spanner snippets by checking the EXPERIMENTAL_HOST environment variable and applying the necessary client options to both the database admin and data clients. Feedback suggests using a more idiomatic Go slice declaration, renaming the environment variable to be more specific (e.g., SPANNER_EXPERIMENTAL_HOST), and replacing log.Fatal calls with error returns to ensure deferred cleanup functions are executed correctly.
| cfg := spanner.ClientConfig{ | ||
| DatabaseRole: databaseRole, | ||
| } | ||
| opts := []option.ClientOption{} |
There was a problem hiding this comment.
In Go, it is more idiomatic to use var for an empty slice that will be appended to, rather than an empty slice literal. This avoids an unnecessary allocation and is generally preferred when the slice's initial capacity is not known.
| opts := []option.ClientOption{} | |
| var opts []option.ClientOption |
References
- In Go, it is more idiomatic to use var for an empty slice that will be appended to, rather than an empty slice literal. (link)
| opts := []option.ClientOption{} | ||
|
|
||
| adminClient, err := database.NewDatabaseAdminClient(ctx) | ||
| endpoint := os.Getenv("EXPERIMENTAL_HOST") |
There was a problem hiding this comment.
The environment variable name EXPERIMENTAL_HOST is quite generic. Consider using a more specific name like SPANNER_EXPERIMENTAL_HOST to avoid potential collisions with other tools or services that might use a similar naming convention.
| cfg.IsExperimentalHost = true | ||
| } | ||
|
|
||
| adminClient, err := database.NewDatabaseAdminClient(ctx, opts...) |
There was a problem hiding this comment.
The run function returns an error, but errors during client creation are handled with log.Fatal, which terminates the process immediately. This prevents deferred functions (like adminClient.Close()) from running and is inconsistent with how other errors in this function are handled. Consider returning the error instead of using log.Fatal.
| defer adminClient.Close() | ||
|
|
||
| dataClient, err := spanner.NewClientWithConfig(ctx, db, cfg) | ||
| dataClient, err := spanner.NewClientWithConfig(ctx, db, cfg, opts...) |
There was a problem hiding this comment.
Similar to the adminClient creation, using log.Fatal here prevents the previously created adminClient from being closed via its deferred Close() call if spanner.NewClientWithConfig fails. It is better to return the error to the caller.
amcolin
left a comment
amcolin
left a comment
There was a problem hiding this comment.
This is a great addition! Once the feedback from Gemini has been addressed, I think this will be in a good spot to merge. Nice work!
3 participants
Description
Fixes #
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)