feat: Cloud Bigtable Managed Backup Implementation - #305
Conversation
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.
|
igorbernstein2
left a comment
There was a problem hiding this comment.
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?
igorbernstein2
left a comment
There was a problem hiding this comment.
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 { |
| } | ||
|
|
||
| @Nullable | ||
| public OptimizeRestoredTableOperationToken getOptimizeRestoredTableOperationToken() { |
There was a problem hiding this comment.
javadoc and in particular note why the token would be null here
| private static String prefix; | ||
|
|
||
| @BeforeClass | ||
| public static void createClient() |
There was a problem hiding this comment.
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.
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.
Does this need to be specified? Can it just be the first cluster in the instance?
There was a problem hiding this comment.
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.
When would we want to change this?
There was a problem hiding this comment.
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: