-
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
feat: Copy backup samples #1802
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
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
samples/snippets/src/test/java/com/example/spanner/SpannerSampleIT.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
@@ -1659,21 +1660,23 @@ static void cancelCreateBackup( | |||
// [END spanner_cancel_backup_create] | |||
|
|||
// [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? |
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
``` | ||
|
||
If you are using SBT, add this to your dependencies | ||
|
||
```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!
google-cloud-spanner/pom.xml
Outdated
@@ -343,7 +343,7 @@ | |||
<dependency> | |||
<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?
samples/install-without-bom/pom.xml
Outdated
@@ -32,7 +32,7 @@ | |||
<dependency> | |||
<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
samples/snippets/pom.xml
Outdated
@@ -33,7 +33,7 @@ | |||
<dependency> | |||
<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)
@@ -1659,21 +1660,23 @@ static void cancelCreateBackup( | |||
// [END spanner_cancel_backup_create] | |||
|
|||
// [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 <[email protected]>
…ample.java Co-authored-by: Knut Olav Løite <[email protected]>
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: