feat: Spanner copy backup ct by ansh0l · Pull Request #1718 · googleapis/java-spanner

ansh0l · 2022-02-28T03:40:47Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

generated-files-bot · 2022-02-28T03:40:53Z

Warning: This pull request is touching the following templated files:

google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClient.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminSettings.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/gapic_metadata.json
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/DatabaseAdminStub.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/DatabaseAdminStubSettings.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/stub/GrpcDatabaseAdminStub.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClientTest.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/MockDatabaseAdminImpl.java
grpc-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/DatabaseAdminGrpc.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/Backup.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/BackupOrBuilder.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/BackupProto.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/SpannerDatabaseAdminProto.java
proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/backup.proto
proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/Instance.java
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/InstanceOrBuilder.java
proto-google-cloud-spanner-admin-instance-v1/src/main/java/com/google/spanner/admin/instance/v1/SpannerInstanceAdminProto.java
proto-google-cloud-spanner-admin-instance-v1/src/main/proto/google/spanner/admin/instance/v1/spanner_instance_admin.proto
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CreateSessionRequest.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CreateSessionRequestOrBuilder.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/ExecuteBatchDmlRequest.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/Mutation.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/MutationProto.java
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java
proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/mutation.proto
proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/spanner.proto

snippet-bot · 2022-02-28T03:41:02Z

Here is the summary of changes.

You are about to add 1 region tag.

samples/snippets/src/main/java/com/example/spanner/CopyBackupSample.java:19, tag spanner_copy_backup

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

thiagotnunes · 2022-02-28T04:05:18Z

+   * OperationFuture<Backup, CopyBackupMetadata> op = dbAdminClient
+   *     .copyBackup(
+   *         instanceId,
+   *         sourceBackup,


nit: sourceBackupId

thiagotnunes · 2022-02-28T04:06:35Z

+   *             "projects/my-project/locations/some-location/keyRings/my-keyring/cryptoKeys/my-key"));
+   *
+   * Backup backupToCopy = dbAdminClient
+   *     .newBackupBuilder(destinationBackupId)


Don't we need a sourceBackupId?

thiagotnunes · 2022-02-28T04:09:33Z

+  public OperationFuture<Backup, CopyBackupMetadata> copyBackup(Backup backupInfo)
+          throws SpannerException {
+    Preconditions.checkArgument(
+            backupInfo.getId() != null, "Cannot create a backup without a source backup");


Isn't the id the destination backup? (from your javadocs?)

We should also check for the destination backup id as you are doing here

thiagotnunes · 2022-02-28T04:12:28Z

@@ -0,0 +1,42 @@
+diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml
+index f5d1351a..9ede6f23 100644


Do we need this file?

thiagotnunes · 2022-02-28T04:13:30Z

    <dependency>
      <groupId>com.google.cloud</groupId>
      <artifactId>google-cloud-spanner</artifactId>
+      <version>6.19.1-SNAPSHOT</version>


I think we added this for testing, but forgot to remove it?

thiagotnunes · 2022-02-28T04:13:36Z

+      <version>6.19.1-SNAPSHOT</version>
    </dependency>
    <!-- [END spanner_install_with_bom] -->
+    <dependency>


thiagotnunes · 2022-02-28T04:14:11Z

+    OperationFuture<Backup, CopyBackupMetadata> op = dbAdminClient.copyBackup(backup);
+    try {
+      // Wait for the backup operation to complete.
+      backup = op.get();


Please add a timeout here (it could be a long one).

thiagotnunes · 2022-02-28T04:16:18Z

+                                .getProgress()
+                                .getStartTime();
+                      } catch (InvalidProtocolBufferException e) {
+                        return null;


returning null makes me worried. Should we throw a RuntimeException?

I think this is OK in this case. We use this to order operations by start time, and this just ensures that if the listBackupOperations happens to return an operation without a start time (or some other invalid operation) that we just ignore it.

thiagotnunes · 2022-02-28T04:21:48Z

+  // any referencing backup prevents the backup from being deleted. When the
+  // copy operation is done (either successfully completed or cancelled or the
+  // destination backup is deleted), the reference to the backup is removed.
+  repeated string referencing_backups = 11 [


Did we expose this?

We need to expose this in the BackupInfo and when reading the response proto we need to map this.

thiagotnunes · 2022-02-28T04:21:56Z

+  // multiple APIs: CreateBackup, UpdateBackup, CopyBackup. When updating or
+  // copying an existing backup, the expiration time specified must be
+  // less than `Backup.max_expire_time`.
+  google.protobuf.Timestamp max_expire_time = 12 [(google.api.field_behavior) = OUTPUT_ONLY];


Did we expose this?

We need to expose this in the BackupInfo and when reading the response proto we need to map this.

thiagotnunes · 2022-02-28T04:22:50Z

+message CopyBackupRequest {
+  // Required. The name of the destination instance that will contain the backup copy.
+  // Values are of the form: `projects/<project>/instances/<instance>`.
+  string parent = 1 [


It seems we need to populate this, did we?

thiagotnunes · 2022-02-28T04:57:07Z

+  // any referencing backup prevents the backup from being deleted. When the
+  // copy operation is done (either successfully completed or cancelled or the
+  // destination backup is deleted), the reference to the backup is removed.
+  repeated string referencing_backups = 11 [


We need to expose this in the BackupInfo and when reading the response proto we need to map this.

thiagotnunes · 2022-02-28T04:57:20Z

+  // multiple APIs: CreateBackup, UpdateBackup, CopyBackup. When updating or
+  // copying an existing backup, the expiration time specified must be
+  // less than `Backup.max_expire_time`.
+  google.protobuf.Timestamp max_expire_time = 12 [(google.api.field_behavior) = OUTPUT_ONLY];


We need to expose this in the BackupInfo and when reading the response proto we need to map this.

thiagotnunes · 2022-02-28T05:12:04Z

+  public OperationFuture<Backup, CopyBackupMetadata> copyBackup(Backup backupInfo)
+          throws SpannerException {
+    Preconditions.checkArgument(
+            backupInfo.getId() != null, "Cannot create a backup without a source backup");


We should also check for the destination backup id as you are doing here

thiagotnunes · 2022-02-28T05:14:32Z

+            CopyBackupRequest.newBuilder()
+                    .setParent(instanceName)
+                    .setBackupId(backupId)
+                    .setSourceBackup(backupId)


This should be sourceBackupId

thiagotnunes · 2022-02-28T05:17:04Z

+            .setSourceBackup(sourceBackupId)
+            .build();
+
+    Timestamp expireTime = Timestamp.ofTimeMicroseconds(TimeUnit.MICROSECONDS.convert(


Use the expireTime in the backup to be copied

thiagotnunes · 2022-02-28T05:18:41Z

+            System.currentTimeMillis() + TimeUnit.DAYS.toMillis(14), TimeUnit.MILLISECONDS));
+
+    // Initiate the requefst which returns an OperationFuture.
+    System.out.println("Copying backup [" + backup.getId() + "]...");


This is the destination backup id, let's print the source backup

thiagotnunes · 2022-02-28T05:19:47Z

            TimeUnit.SECONDS.toMicros(backup.getExpireTime().getSeconds())
                + TimeUnit.NANOSECONDS.toMicros(backup.getExpireTime().getNanos())
                + TimeUnit.DAYS.toMicros(30L));
+    int timeDiff = expireTime.compareTo(backup.getExpireTime());


This can be removed for now

olavloite · 2022-02-28T08:12:16Z

+   *
+   * @param sourceInstanceId the id of the instance where the database to backup is located and
+   *     where the backup will be created.
+   * @param sourceBackup the source backup id.


nit: here also sourceBackupId to be consistent

olavloite · 2022-02-28T08:17:39Z

+    final String instanceName = backupInfo.getInstanceId().getName();
+    final String databaseName = backupInfo.getDatabase().getName();
+    final String backupId = backupInfo.getId().getBackup();
+    final Backup.Builder backupBuilder =


I don't think we are actually using this builder anywhere. Could we remove it?

olavloite · 2022-02-28T08:19:44Z

+                                .getProgress()
+                                .getStartTime();
+                      } catch (InvalidProtocolBufferException e) {
+                        return null;


I think this is OK in this case. We use this to order operations by start time, and this just ensures that if the listBackupOperations happens to return an operation without a start time (or some other invalid operation) that we just ignore it.

olavloite · 2022-02-28T08:20:05Z

+  OperationFuture<Backup, CopyBackupMetadata> copyBackUp(
+          com.google.cloud.spanner.Backup backupInfo) throws SpannerException;
+
+


nit: remove extra empty line

olavloite · 2022-02-28T08:22:06Z

-            .setEncryptionInfo(ENCRYPTION_INFO)
-            .setState(com.google.spanner.admin.database.v1.Backup.State.CREATING)
-            .build();
+            com.google.spanner.admin.database.v1.Backup.newBuilder()


More of a question than an action item, but: Do you know why the formatter is changing the indentation here? Are you just using the normal mvn com.coveo:fmt-maven-plugin:format command, or is this a new format plugin?

feat: max expire time and referencing backup

ansh0l · 2022-03-25T03:35:27Z

Closing this PR in the interest of https://github.com/googleapis/java-spanner/pull/1778/files (which contains fixes by @asthamohta and removes autogenerated code)

chore: upgrade native image checks to graalvm-22.3.0 Source-Link: googleapis/synthtool@5e52896 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:27b1b1884dce60460d7521b23c2a73376cba90c0ef3d9f0d32e4bdb786959cfd

chore: upgrade native image checks to graalvm-22.3.0 Source-Link: googleapis/synthtool@5e52896 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:27b1b1884dce60460d7521b23c2a73376cba90c0ef3d9f0d32e4bdb786959cfd Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

ansh0l added 10 commits February 18, 2022 14:43

feat: copy backup

5703628

feat: copy backup

0bfe894

feat: copy backup - porting code changes

6f9532e

feat: copy backup - porting partial sample

03dcbf2

feat: copy backup - cleaning up tests

3ecb63a

feat: copy backup - cleaning up samples

582d82e

feat: copy backup - cleaning up samples

5ed9e33

feat: copy backup signature fixes

bf58bd3

feat: copy backup sample fixes

0fd8883

feat: copy backup - review fixes

e2848fe

ansh0l requested review from a team February 28, 2022 03:40

product-auto-label Bot added the api: spanner Issues related to the googleapis/java-spanner API. label Feb 28, 2022

ansh0l added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 28, 2022

ansh0l changed the title ~~Spanner copy backup ct~~ feat: Spanner copy backup ct Feb 28, 2022

feat: copy backup checkstyle fixes

f7d3909

thiagotnunes reviewed Feb 28, 2022

View reviewed changes

olavloite reviewed Feb 28, 2022

View reviewed changes

olavloite and others added 7 commits February 28, 2022 17:02

feat: make CopyBackup sample runnable

986f2f2

fix: checkstyle violation

0ecbeb7

feat: adding max expire time and get referencing database support

8748972

samples: adding copy backup operation support

5ce7a2b

adding documentation

2470206

linting changes

fad5c0a

Merge pull request #1 from asthamohta/cross-region-backup

4a4de36

feat: max expire time and referencing backup

ansh0l closed this Mar 25, 2022

		@@ -0,0 +1,42 @@
		diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml
		index f5d1351a..9ede6f23 100644

		OperationFuture<Backup, CopyBackupMetadata> copyBackUp(
		com.google.cloud.spanner.Backup backupInfo) throws SpannerException;

Conversation

ansh0l commented Feb 28, 2022

Uh oh!

generated-files-bot Bot commented Feb 28, 2022

Uh oh!

snippet-bot Bot commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansh0l commented Mar 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snippet-bot Bot commented Feb 28, 2022 •

edited

Loading