-
Notifications
You must be signed in to change notification settings - Fork 135
feat: Copy Backup Support #1778
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
|
Warning: This pull request is touching the following templated files:
|
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
48d840d to
2d627aa
Compare
thiagotnunes
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.
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.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
| * | ||
| * <p>Returns the names of the destination backups being created by copying this source backup. | ||
| */ | ||
| protected abstract Builder setReferencingBackup(ProtocolStringList referencingBackup); |
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.
We should not use the protobuf specific types here. Instead of ProtocolStringList, could you use List<String>?
google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
Outdated
Show resolved
Hide resolved
thiagotnunes
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.
We need to resolve the breaking changes, populate the new fields when reading the proto and remove the samples
a28dc35 to
10d2134
Compare
10d2134 to
b06a3df
Compare
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClient.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
a4d44b2 to
809465c
Compare
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java
Outdated
Show resolved
Hide resolved
| .setDatabase(DatabaseId.of(proto.getDatabase())) | ||
| .setEncryptionInfo(EncryptionInfo.fromProtoOrNull(proto.getEncryptionInfo())) | ||
| .setProto(proto) | ||
| .setMaxExpireTime(Timestamp.fromProto(proto.getMaxExpireTime())) |
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.
Did we test with null max expire time and null referencingBackupsList?
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.
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
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.
I will remove my request changes so you are not blocked by my review, but please address the method name change
cfc43a8 to
a968222
Compare
No description provided.