-
Notifications
You must be signed in to change notification settings - Fork 136
feat: Copy backup samples #1802
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
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
c370f35 to
e6d042b
Compare
olavloite
left a comment
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.
There's no integration test for the CopyBackup sample. Is that intentional?
samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/SpannerSample.java
Outdated
Show resolved
Hide resolved
|
@olavloite yes, because I notices not all samples have an IT and this is a very long running function so decided to go with this |
samples/snippets/src/test/java/com/example/spanner/SpannerSampleIT.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java
Outdated
Show resolved
Hide resolved
samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java
Outdated
Show resolved
Hide resolved
|
|
||
| // [START spanner_list_backup_operations] | ||
| static void listBackupOperations(InstanceAdminClient instanceAdminClient, DatabaseId databaseId) { | ||
| static void listBackupOperations( |
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.
Isn't this a breaking change? Maybe customers won't care because it is in the sample , but I don't know what the policy is here. Yoshi team might know
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.
@olavloite pointed out that this is a non public method and hence can't be called so this will not be a breaking change
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.
That is true, but we publish these samples in our public documentation, so we would be changing the signature. Anyway, if your team is ok with this, I am ok.
|
@asthamohta : can you update the PR to provide the description in the right format? |
ansh0l
left a comment
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.
Update the PR description
f36e29f to
474dfa8
Compare
README.md
Outdated
|
|
||
| ```Scala | ||
| libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.23.0" | ||
| libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.23.2" |
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: I don't think these version updates should be part of this PR
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.
So I didn't do this. After I raised the PR, the changes in this file came automatically after I raised the PR. I only changes the install-without-bom. I'll try merging the other PR with the version change and remove these files
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.
Thanks!
| <groupId>org.openjdk.jmh</groupId> | ||
| <artifactId>jmh-core</artifactId> | ||
| <version>1.34</version> | ||
| <version>1.35</version> |
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.
This should happen automatically no? Why do we need to update jmh here?
| <groupId>com.google.cloud</groupId> | ||
| <artifactId>google-cloud-spanner</artifactId> | ||
| <version>6.21.2</version> | ||
| <version>6.23.2</version> |
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.
This is already part of this PR: https://github.com/googleapis/java-spanner/pull/1788/files
| <groupId>com.google.cloud</groupId> | ||
| <artifactId>libraries-bom</artifactId> | ||
| <version>25.0.0</version> | ||
| <version>25.1.0</version> |
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.
This should happen automatically in another PR (from dependabot)
|
|
||
| // [START spanner_list_backup_operations] | ||
| static void listBackupOperations(InstanceAdminClient instanceAdminClient, DatabaseId databaseId) { | ||
| static void listBackupOperations( |
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.
That is true, but we publish these samples in our public documentation, so we would be changing the signature. Anyway, if your team is ok with this, I am ok.
…le.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…ample.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
This reverts commit cdf4d17.
d9d234b to
0920a95
Compare
| OffsetDateTime.now().getOffset()).toString())); | ||
| expireTime.getSeconds(), | ||
| expireTime.getNanos(), | ||
| OffsetDateTime.now().getOffset()).toString())); |
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.
super nit: the .toString() is superfluous
Adding the following samples for Copy Backup: