feat: Spanner copy backup ct #1718
Conversation
product-auto-label
bot
added
the
api: spanner
|
Warning: This pull request is touching the following templated files:
|
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
ansh0l
added
the
do not merge
| * OperationFuture<Backup, CopyBackupMetadata> op = dbAdminClient | ||
| * .copyBackup( | ||
| * instanceId, | ||
| * sourceBackup, |
| * "projects/my-project/locations/some-location/keyRings/my-keyring/cryptoKeys/my-key")); | ||
| * | ||
| * Backup backupToCopy = dbAdminClient | ||
| * .newBackupBuilder(destinationBackupId) |
There was a problem hiding this comment.
Don't we need a sourceBackupId?
| public OperationFuture<Backup, CopyBackupMetadata> copyBackup(Backup backupInfo) | ||
| throws SpannerException { | ||
| Preconditions.checkArgument( | ||
| backupInfo.getId() != null, "Cannot create a backup without a source backup"); |
There was a problem hiding this comment.
Isn't the id the destination backup? (from your javadocs?)
There was a problem hiding this comment.
We should also check for the destination backup id as you are doing here
| @@ -0,0 +1,42 @@ | |||
| diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml | |||
| index f5d1351a..9ede6f23 100644 | |||
There was a problem hiding this comment.
Do we need this file?
| @@ -44,8 +44,14 @@ | |||
| <dependency> | |||
| <groupId>com.google.cloud</groupId> | |||
| <artifactId>google-cloud-spanner</artifactId> | |||
| <version>6.19.1-SNAPSHOT</version> | |||
There was a problem hiding this comment.
I think we added this for testing, but forgot to remove it?
| </dependency> | ||
| <!-- [END spanner_install_with_bom] --> | ||
| <dependency> |
| OperationFuture<Backup, CopyBackupMetadata> op = dbAdminClient.copyBackup(backup); | ||
| try { | ||
| // Wait for the backup operation to complete. | ||
| backup = op.get(); |
There was a problem hiding this comment.
Please add a timeout here (it could be a long one).
| .getProgress() | ||
| .getStartTime(); | ||
| } catch (InvalidProtocolBufferException e) { | ||
| return null; |
There was a problem hiding this comment.
returning null makes me worried. Should we throw a RuntimeException?
There was a problem hiding this comment.
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.
| // 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 [ |
There was a problem hiding this comment.
We need to expose this in the BackupInfo and when reading the response proto we need to map this.
| // 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]; |
There was a problem hiding this comment.
We need to expose this in the BackupInfo and when reading the response proto we need to map this.
| 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 [ |
There was a problem hiding this comment.
It seems we need to populate this, did we?
| // 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 [ |
There was a problem hiding this comment.
We need to expose this in the BackupInfo and when reading the response proto we need to map this.
| // 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]; |
There was a problem hiding this comment.
We need to expose this in the BackupInfo and when reading the response proto we need to map this.
| public OperationFuture<Backup, CopyBackupMetadata> copyBackup(Backup backupInfo) | ||
| throws SpannerException { | ||
| Preconditions.checkArgument( | ||
| backupInfo.getId() != null, "Cannot create a backup without a source backup"); |
There was a problem hiding this comment.
We should also check for the destination backup id as you are doing here
| CopyBackupRequest.newBuilder() | ||
| .setParent(instanceName) | ||
| .setBackupId(backupId) | ||
| .setSourceBackup(backupId) |
There was a problem hiding this comment.
This should be sourceBackupId
| .setSourceBackup(sourceBackupId) | ||
| .build(); | ||
|
|
||
| Timestamp expireTime = Timestamp.ofTimeMicroseconds(TimeUnit.MICROSECONDS.convert( |
There was a problem hiding this comment.
Use the expireTime in the backup to be copied
| System.currentTimeMillis() + TimeUnit.DAYS.toMillis(14), TimeUnit.MILLISECONDS)); | ||
|
|
||
| // Initiate the requefst which returns an OperationFuture. | ||
| System.out.println("Copying backup [" + backup.getId() + "]..."); |
There was a problem hiding this comment.
This is the destination backup id, let's print the source backup
| @@ -1840,6 +1887,9 @@ static void updateBackup(DatabaseAdminClient dbAdminClient, BackupId backupId) { | |||
| TimeUnit.SECONDS.toMicros(backup.getExpireTime().getSeconds()) | |||
| + TimeUnit.NANOSECONDS.toMicros(backup.getExpireTime().getNanos()) | |||
| + TimeUnit.DAYS.toMicros(30L)); | |||
| int timeDiff = expireTime.compareTo(backup.getExpireTime()); | |||
There was a problem hiding this comment.
This can be removed for now
| * | ||
| * @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. |
There was a problem hiding this comment.
nit: here also sourceBackupId to be consistent
| final String instanceName = backupInfo.getInstanceId().getName(); | ||
| final String databaseName = backupInfo.getDatabase().getName(); | ||
| final String backupId = backupInfo.getId().getBackup(); | ||
| final Backup.Builder backupBuilder = |
There was a problem hiding this comment.
I don't think we are actually using this builder anywhere. Could we remove it?
| .getProgress() | ||
| .getStartTime(); | ||
| } catch (InvalidProtocolBufferException e) { | ||
| return null; |
There was a problem hiding this comment.
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.
| OperationFuture<Backup, CopyBackupMetadata> copyBackUp( | ||
| com.google.cloud.spanner.Backup backupInfo) throws SpannerException; | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra empty line
| .setEncryptionInfo(ENCRYPTION_INFO) | ||
| .setState(com.google.spanner.admin.database.v1.Backup.State.CREATING) | ||
| .build(); | ||
| com.google.spanner.admin.database.v1.Backup.newBuilder() |
There was a problem hiding this comment.
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
|
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>
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:
Fixes #<issue_number_goes_here> ☕️