-
Notifications
You must be signed in to change notification settings - Fork 123
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
docs: samples and tests for auto-generated createDatabase and createInstance APIs. #2764
Conversation
…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206
fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.
* limitations under the License. | ||
*/ | ||
|
||
package com.example.spanner.v2; |
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.
I think that it is going to be confusing if we use .v2
as a package name for these samples, as the actual change is that the code will start using classes that are in a package called .v1
, while they before that used classes that were in packages without a version number.
Or could we temporarily use this .v2
package, and when we make the switch, move these samples into com.example.spanner
and the old samples into com.example.spanner.deprecated
or something like that?
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.
Yeah as you described, I plan to use this path temporarily. Also instead of calling it .v2
, I changed it to .admin.generated
. Instead of calling the new path deprecated
we can name it archived
. So, the new samples will be moved into com.example.spanner
and the old samples into com.example.spanner.archived
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.
👍
...les/snippets/src/main/java/com/example/spanner/v2/CreateDatabaseWithDefaultLeaderSample.java
Outdated
Show resolved
Hide resolved
...les/snippets/src/main/java/com/example/spanner/v2/CreateDatabaseWithDefaultLeaderSample.java
Outdated
Show resolved
Hide resolved
...les/snippets/src/main/java/com/example/spanner/v2/CreateDatabaseWithDefaultLeaderSample.java
Outdated
Show resolved
Hide resolved
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
…tabaseWithDefaultLeaderSample.java Co-authored-by: Knut Olav Løite <[email protected]>
...src/main/java/com/example/spanner/admin/generated/CreateDatabaseWithDefaultLeaderSample.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/admin/generated/CreateInstanceExample.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/test/java/com/example/spanner/admin/generated/SampleTestBaseV2.java
Show resolved
Hide resolved
I think it looks good from my end. One last thing came to mind when looking at the samples (which may or may not be a concern for Spanner). In google-cloud-java, we had some issues with creating instances for certain services. The project were running the code in had some quotas and the tests/samples had some issues properly deleting the created instances. We just ended up prefixing the resources with something like |
// TODO(developer): Replace these variables before running the sample. | ||
final String instanceName = "projects/my-project/instances/my-instance-id"; | ||
final String databaseId = "my-database-name"; | ||
final String defaultLeader = "my-default-leader"; |
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.
nit: Could we use a default leader name here that is more similar to the actual value that a user would have to use. That might help users who don't know what the default leader option is, to understand and learn that.
samples/snippets/src/test/java/com/example/spanner/admin/generated/SampleTestBaseV2.java
Show resolved
Hide resolved
} | ||
|
||
databaseAdminClient.close(); | ||
instanceAdminClient.close(); |
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.
The generated (admin) clients have both a close()
and a shutdown()
method. Do they do the same (or put another way: Does close()
actually shutdown the client)?
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.
I don't think so. @lqiu96 Would you have a quick answer for this?
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.
I believe they're pretty much the same. Closing the client will close the underlying stub which closes all the BackgroundResources associated with the client. These are resources like channels, watchdog service, executors, etc.
The underlying implementation for the close()
is to call shutdown()
and I believe close()
was originally offered as a way to utilize the client in try-with-resources
🤖 I have created a release *beep* *boop* --- ## [6.56.0](https://togithub.com/googleapis/java-spanner/compare/v6.55.0...v6.56.0) (2024-01-05) ### Features * Add autoscaling config in the instance to support autoscaling in systests ([#2756](https://togithub.com/googleapis/java-spanner/issues/2756)) ([99ae565](https://togithub.com/googleapis/java-spanner/commit/99ae565c5e90a2862b4f195fe64656ba8a05373d)) * Add support for Directed Read options ([#2766](https://togithub.com/googleapis/java-spanner/issues/2766)) ([26c6c63](https://togithub.com/googleapis/java-spanner/commit/26c6c634b685bce66ce7caf05057a98e9cc6f5dc)) * Update OwlBot.yaml file to pull autogenerated executor code ([#2754](https://togithub.com/googleapis/java-spanner/issues/2754)) ([20562d4](https://togithub.com/googleapis/java-spanner/commit/20562d4d7e62ab20bb1c4e78547b218a9a506f21)) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.21.0 ([#2772](https://togithub.com/googleapis/java-spanner/issues/2772)) ([173f520](https://togithub.com/googleapis/java-spanner/commit/173f520f931073c4c6ddf3b3d98d255fb575914f)) ### Documentation * Samples and tests for auto-generated createDatabase and createInstance APIs. ([#2764](https://togithub.com/googleapis/java-spanner/issues/2764)) ([74a586f](https://togithub.com/googleapis/java-spanner/commit/74a586f8713ef742d65400da8f04a750316faf78)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Notes