Skip to content
This repository was archived by the owner on Apr 7, 2026. It is now read-only.

feat: Copy Backup Support#1778

Merged
asthamohta merged 21 commits intogoogleapis:mainfrom
asthamohta:copy-backup-final
Mar 28, 2022
Merged

feat: Copy Backup Support#1778
asthamohta merged 21 commits intogoogleapis:mainfrom
asthamohta:copy-backup-final

Conversation

@asthamohta
Copy link
Copy Markdown
Contributor

@asthamohta asthamohta commented Mar 24, 2022

No description provided.

@asthamohta asthamohta requested review from a team March 24, 2022 08:52
@generated-files-bot
Copy link
Copy Markdown

generated-files-bot Bot commented Mar 24, 2022

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

  • samples/pom.xml

@product-auto-label product-auto-label Bot added api: spanner Issues related to the googleapis/java-spanner API. samples Issues that are directly related to samples. labels Mar 24, 2022
@snippet-bot
Copy link
Copy Markdown

snippet-bot Bot commented Mar 24, 2022 •
edited
Loading

No region tags are edited in this PR.

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

@asthamohta asthamohta requested review from ansh0l and olavloite March 24, 2022 08:56
thiagotnunes
thiagotnunes reviewed Mar 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful when accepting my code suggestions, as you need to change the commit title, otherwise the conventionalcommits check will fail. Feel free to, instead of accepting the suggestions, implementing the same in your local machine.

Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
*
* <p>Returns the names of the destination backups being created by copying this source backup.
*/
protected abstract Builder setReferencingBackup(ProtocolStringList referencingBackup);
Copy link
Copy Markdown
Contributor

@thiagotnunes thiagotnunes Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use the protobuf specific types here. Instead of ProtocolStringList, could you use List<String>?

Comment thread google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java
Comment thread samples/pom.xml Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java Outdated
thiagotnunes
thiagotnunes suggested changes Mar 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to resolve the breaking changes, populate the new fields when reading the proto and remove the samples

@asthamohta asthamohta changed the title feat: Copy Backup support and samples feat: Copy Backup support Mar 25, 2022
@asthamohta asthamohta requested a review from thiagotnunes March 25, 2022 07:54
@asthamohta asthamohta added owlbot:run Add this label to trigger the Owlbot post processor. and removed api: spanner Issues related to the googleapis/java-spanner API. labels Mar 25, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 25, 2022
@asthamohta asthamohta added owlbot:run Add this label to trigger the Owlbot post processor. api: spanner Issues related to the googleapis/java-spanner API. and removed samples Issues that are directly related to samples. labels Mar 25, 2022
olavloite
olavloite approved these changes Mar 25, 2022
View reviewed changes
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
rajatbhatta
rajatbhatta reviewed Mar 25, 2022
View reviewed changes
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java Outdated
@asthamohta asthamohta changed the title feat: Copy Backup support feat: Copy Backup Support Mar 25, 2022
thiagotnunes
thiagotnunes reviewed Mar 25, 2022
View reviewed changes
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java Outdated
Comment thread google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java
.setDatabase(DatabaseId.of(proto.getDatabase()))
.setEncryptionInfo(EncryptionInfo.fromProtoOrNull(proto.getEncryptionInfo()))
.setProto(proto)
.setMaxExpireTime(Timestamp.fromProto(proto.getMaxExpireTime()))
Copy link
Copy Markdown
Contributor

@thiagotnunes thiagotnunes Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we test with null max expire time and null referencingBackupsList?

Copy link
Copy Markdown
Contributor Author

@asthamohta asthamohta Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't get a null value over here. The addAll function checks for non null values-> https://github.com/protocolbuffers/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/AbstractMessageLite.java#L408

thiagotnunes
thiagotnunes reviewed Mar 25, 2022
View reviewed changes
Comment thread google-cloud-spanner/clirr-ignored-differences.xml Outdated
thiagotnunes
thiagotnunes reviewed Mar 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove my request changes so you are not blocked by my review, but please address the method name change

@asthamohta asthamohta added owlbot:run Add this label to trigger the Owlbot post processor. and removed owlbot:run Add this label to trigger the Owlbot post processor. labels Mar 28, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2022
@asthamohta asthamohta merged commit dc79366 into googleapis:main Mar 28, 2022
@release-please release-please Bot mentioned this pull request Mar 28, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ansh0l ansh0l Awaiting requested review from ansh0l

3 more reviewers

@olavloite olavloite olavloite approved these changes

@thiagotnunes thiagotnunes thiagotnunes approved these changes

@rajatbhatta rajatbhatta rajatbhatta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lesv lesv

Labels

api: spanner Issues related to the googleapis/java-spanner API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@asthamohta @thiagotnunes @olavloite @rajatbhatta @lesv @ansh0l