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: Cloud Bigtable Managed Backup Implementation #305
Conversation
...igtable/src/test/java/com/google/cloud/bigtable/admin/v2/models/UpdateBackupRequestTest.java
Outdated
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminClient.java
Outdated
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminClient.java
Show resolved
Hide resolved
...e-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/internal/NameUtilTest.java
Show resolved
Hide resolved
...igtable/src/test/java/com/google/cloud/bigtable/admin/v2/models/RestoreTableRequestTest.java
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminClient.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
============================================
+ Coverage 79.51% 80.24% +0.73%
- Complexity 991 1102 +111
============================================
Files 99 105 +6
Lines 6444 6815 +371
Branches 318 364 +46
============================================
+ Hits 5124 5469 +345
- Misses 1119 1148 +29
+ Partials 201 198 -3
Continue to review full report at Codecov.
|
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.
Can you add javadoc for all of the public surface?
- client methods
- models
And for the client methods, can you add some sample code to illustrate its intended usage?
...loud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/BigtableTableAdminClient.java
Outdated
Show resolved
Hide resolved
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/Backup.java
Outdated
Show resolved
Hide resolved
google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/Backup.java
Outdated
Show resolved
Hide resolved
...main/java/com/google/cloud/bigtable/admin/v2/models/OptimizeRestoredTableOperationToken.java
Outdated
Show resolved
Hide resolved
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 think this can be merged as is, but I think it needs some follow up work (the integration tests were written before the testenv stuff existed, so should be migrated)
import com.google.common.base.Strings; | ||
import javax.annotation.Nullable; | ||
|
||
public class RestoredTableResult { |
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.
javadoc please
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.
Done.
} | ||
|
||
@Nullable | ||
public OptimizeRestoredTableOperationToken getOptimizeRestoredTableOperationToken() { |
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.
javadoc and in particular note why the token would be null here
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.
Done.
private static String prefix; | ||
|
||
@BeforeClass | ||
public static void createClient() |
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.
The integration tests should use the existing testing infrastructure:
https://github.com/googleapis/java-bigtable/tree/master/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/test_helpers/env
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.
Discussed with @kolea2 offline and this will left in future PRs.
missingProperties.add(INSTANCE_PROPERTY_NAME); | ||
} | ||
|
||
targetCluster = System.getProperty(CLUSTER_PROPERTY_NAME); |
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.
Does this need to be specified? Can it just be the first cluster in the instance?
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 discussed this with @kolea2 and integration test improvements will be in separate PRs.
adminApiEndpoint = "bigtableadmin.googleapis.com:443"; | ||
} | ||
|
||
int tableSize = MoreObjects.firstNonNull(Integer.getInteger(TABLE_SIZE_PROPERTY_NAME), 1); |
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.
When would we want to change this?
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.
Please see my other comment. I discussed this with @kolea2 and integration test improvements will be in separate PRs.
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: