Skip to content
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 API support #1398

Merged
merged 22 commits into from
Aug 17, 2023
Merged

feat: copy backup API support #1398

merged 22 commits into from
Aug 17, 2023

Conversation

TracyCuiCan
Copy link
Contributor

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> ☕️

If you write sample code, please follow the samples format.

@TracyCuiCan TracyCuiCan requested review from a team as code owners September 15, 2022 20:38
@generated-files-bot
Copy link

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

  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BaseBigtableTableAdminClient.java
  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BaseBigtableTableAdminSettings.java
  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/stub/BigtableTableAdminStub.java
  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/stub/BigtableTableAdminStubSettings.java
  • google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/stub/GrpcBigtableTableAdminStub.java
  • google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/BaseBigtableTableAdminClientTest.java
  • google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/MockBigtableTableAdminImpl.java
  • grpc-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/BigtableTableAdminGrpc.java
  • proto-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/Backup.java
  • proto-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/BackupInfo.java
  • proto-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/BackupInfoOrBuilder.java
  • proto-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/BackupOrBuilder.java
  • proto-google-cloud-bigtable-admin-v2/src/main/java/com/google/bigtable/admin/v2/BigtableTableAdminProto.java
  • proto-google-cloud-bigtable-admin-v2/src/main/proto/google/bigtable/admin/v2/bigtable_table_admin.proto
  • proto-google-cloud-bigtable-admin-v2/src/main/proto/google/bigtable/admin/v2/table.proto

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/java-bigtable API. labels Sep 15, 2022
@@ -0,0 +1,408 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that the same change was committed 12 days ago to https://github.com/googleapis/java-bigtable/blob/main/google-cloud-bigtable-stats/pom.xml. This was a valid change back then and should be no change now.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the dependency-reduced-pom.xml which gets generated (as opposed to pom.xml) and it should be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

(separately we should figure out how to exclude this - maybe in .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I'll remove this file from the PR. Will figure out how to exclude this and make changes in a different PR, sounds good?

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect, thanks!

@@ -0,0 +1,408 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the dependency-reduced-pom.xml which gets generated (as opposed to pom.xml) and it should be reverted.

* #of(String, String, String, String) of} method
*/
public static CopyBackupRequest of(String clusterId, String backupId) {
CopyBackupRequest request = new CopyBackupRequest(null, null, clusterId, backupId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to match up - clusterId is the 3rd parameter, but the 3rd parameter should be instanceId per the method signature: https://github.com/googleapis/java-bigtable/pull/1398/files#diff-f22bea44852cc477c88df0651b15890d220ccbfdf7f998d0e97bc6c30d409b8bR65

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The private method is defined as:
private CopyBackupRequest(
@nullable String sourceProjectId,
@nullable String sourceInstanceId,
@nonnull String sourceClusterId,
@nonnull String sourceBackupId);
The 3rd parameter is ClusterId, the order of parameters of the private method and the public ones are different, should I match them?

Copy link
Contributor

Choose a reason for hiding this comment

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

got it - in this case, the private method should follow the order of the public ones to avoid confusion (like I just had :)). It will also change the order so the null parameters will go toward the end, which is more typical and expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sg, I'll udpate!

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Oct 20, 2022
kolea2
kolea2 previously approved these changes Oct 24, 2022
Copy link
Contributor

@kolea2 kolea2 left a comment

Choose a reason for hiding this comment

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

lgtm with just some last suggestions on method and variable naming. Thanks for putting this together!

@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2022
@kolea2 kolea2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 29, 2022
@kolea2 kolea2 dismissed their stale review August 16, 2023 13:06

waiting on update/ review from Igor

@igorbernstein2 igorbernstein2 added automerge Merge the pull request once unit tests and other checks pass. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 17, 2023
@mutianf mutianf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2023
@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 17, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 17, 2023
@mutianf mutianf merged commit 558a408 into googleapis:main Aug 17, 2023
16 of 17 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants