Bigtable: Introducing new Batching API for Bigtable by rahulKQL · Pull Request #6133 · googleapis/google-cloud-java

rahulKQL · 2019-08-22T11:24:48Z

Working towards #3003

Introducing new API BigtableDataClient#newBulkMutationBatcher(String).
Deprecated existing API BigtableDataClient#newBulkMutationBatcher().
Updated BatchingCallSettings to be compatible with new Batching API.
Removed description from deprecated methods/classes.
Added a new model in RowMutationEntry.
Added MutateRowsBatchingDescriptorV2 to spool the batching response.
Updated JavaDocs, test cases according to latest changes.

igorbernstein2 · 2019-08-22T15:15:30Z

Can you investigate why the linkage monitor is complaining?

rahulKQL · 2019-08-22T15:18:02Z

It seems the linkage monitor build was not able to find new BatchingDescirptor, Although the build was successful. I will update more information here after checking again.

codecov · 2019-08-22T17:47:53Z

Codecov Report

Merging #6133 into master will increase coverage by 0.01%.
The diff coverage is 87.73%.

@@ Coverage Diff @@## master #6133 +/- ##
============================================
+ Coverage 47.5% 47.51% +0.01% - Complexity 27447 27460 +13 
============================================
Files 2523 2526 +3 Lines 274592 274691 +99 Branches 31396 31405 +9 ============================================
+ Hits 130445 130531 +86 - Misses 134514 134527 +13 
Partials 9633 9633

Impacted Files	Coverage Δ	Complexity Δ
...d/bigtable/data/v2/models/BulkMutationBatcher.java	`100% <ø> (ø)`	`11 <0> (ø)`	⬇️
...ud/bigtable/data/v2/stub/EnhancedBigtableStub.java	`94.85% <0%> (-3.63%)`	`22 <0> (ø)`
...ble/data/v2/stub/EnhancedBigtableStubSettings.java	`98.7% <100%> (+3.7%)`	`17 <0> (ø)`	⬇️
...le/cloud/bigtable/data/v2/models/BulkMutation.java	`100% <100%> (ø)`	`9 <2> (+2)`	⬆️
...ble/data/v2/stub/BigtableBatchingCallSettings.java	`100% <100%> (ø)`	`5 <5> (?)`
...gle/cloud/bigtable/data/v2/BigtableDataClient.java	`89.79% <100%> (+0.21%)`	`33 <1> (+1)`	⬆️
...tub/mutaterows/MutateRowsBatchingDescriptorV2.java	`100% <100%> (ø)`	`11 <11> (?)`
...loud/bigtable/data/v2/models/RowMutationEntry.java	`74.19% <74.19%> (ø)`	`9 <9> (?)`
.../java/com/google/cloud/testing/CommandWrapper.java	`87.87% <0%> (-9.1%)`	`13% <0%> (ø)`
.../cloud/datastore/testing/LocalDatastoreHelper.java	`80.59% <0%> (-4.48%)`	`17% <0%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b188bf3...98c81e4. Read the comment docs.

rahulKQL · 2019-08-27T09:55:33Z

@igorbernstein2 @kolea2 I have added BigtableBatchingCallSettings, Please have a look and let me know your thoughts on this.

- Introducing new API `BigtableDataClient#newBulkMutationBatcher(String)`. - Deprecated existing API `BigtableDataClient#newBulkMutationBatcher()`. - Updated BatchingCallSettings to be compatible with new Batching API. - Removed method/class description from deprecation. - Added a new model in RowMutationEntry. - Added MutateRowsBatchingDescriptorV2 to spool the batching response. - Updated JavaDocs, test cases according to latest changes.

- Fix for linkage error on Javadocs - Introduced BigtableBatchingCallSettings which will wrap the underlying gax's BatchingCallSettings. This was done for enabling the bigtable throttling to balance the bigtable clusters performance supposedly for batch jobs while still supporting the live load. - Added null check message for RowMutationEntry - Updated javadoc and access level of getter BigtableBatchingCallSettings

This is a trial fix, as linkage monitor does not seems to be recreated with local build

igorbernstein2

LGTM

kolea2 · 2019-08-27T15:58:20Z

@chingor13 is the linkage build failure because the libraries-bom needs to upgrade gax (through the google-cloud-java bom)? Can we exclude this from the check for now?

rahulKQL · 2019-08-27T17:39:13Z

The linkage build is failing for below classes which introduced in gax-1.48.0, and the libraries-bom-2.2.1 points to gax-1.44.0.

BOM Coordinates: com.google.cloud:libraries-bom:2.2.1
Newly introduced problems:
Class com.google.api.gax.batching.BatchingDescriptor is not found
referenced from com.google.cloud.bigtable.data.v2.stub.BigtableBatchingCallSettings (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
referenced from com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsBatchingDescriptorV2 (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
Class com.google.api.gax.batching.BatchingRequestBuilder is not found
referenced from com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsBatchingDescriptorV2 (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
Class com.google.api.gax.batching.BatcherImpl is not found
referenced from com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStub (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
Class com.google.api.gax.batching.BatchingCallSettings is not found
referenced from com.google.cloud.bigtable.data.v2.stub.BigtableBatchingCallSettings (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
Class com.google.api.gax.batching.BatchingCallSettings$Builder is not found
referenced from com.google.cloud.bigtable.data.v2.stub.BigtableBatchingCallSettings (google-cloud-bigtable-0.106.1-SNAPSHOT.jar)
Exception in thread "main" com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitorException: Found 5 new linkage errors
at com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitor.run(LinkageMonitor.java:105)
at com.google.cloud.tools.dependencies.linkagemonitor.LinkageMonitor.main(LinkageMonitor.java:63)

igorbernstein2 · 2019-08-27T18:21:54Z

@kolea2 is looking into it, she said that she'll bring it up with @elharo

elharo · 2019-08-27T18:25:29Z

gax is not part of libraries-bom so it can't be updated there. It only gets pulled in transitively from other dependencies. I don't recommend turning off this check or excluding it. It's likely telling us somehting important that we should address.

elharo · 2019-08-27T18:31:16Z

Based in the report, this PR seems to lead to user-visible linkage errors because you're using new classes in the latest GAX that are not available in older ones, and those older versions are likely to get pulled into clients' classpaths.

igorbernstein2 · 2019-08-27T18:49:11Z

What is the suggested path for adding new features to gax & using them? Why is the gax version referenced by google-cloud-bom 0.99.0 considered the golden?

igorbernstein2 · 2019-08-27T18:49:47Z

Also please note that bigtable is not GA yet and the binary compatibility breakage is expected

elharo · 2019-08-27T19:16:37Z

OK, looks like gax is pulled in from google-cloud-bom. libraries-bom has an older verison of google-cloud-bom due to incompatibilities with protobuf. That might be resolved with the next protobuf release, tenatively at the end of this week.

kolea2 · 2019-08-27T20:24:35Z

As discussed, we will merge this in for now while we wait for the libraries-bom to be updated. I retriggered the failed windows build, waiting for that to pass.

Note changes to the bulk mutation batcher: googleapis/google-cloud-java#6133

* Introducing new Batching API for Bigtable - Introducing new API `BigtableDataClient#newBulkMutationBatcher(String)`. - Deprecated existing API `BigtableDataClient#newBulkMutationBatcher()`. - Updated BatchingCallSettings to be compatible with new Batching API. - Removed method/class description from deprecation. - Added a new model in RowMutationEntry. - Added MutateRowsBatchingDescriptorV2 to spool the batching response. - Updated JavaDocs, test cases according to latest changes. * Addressing minor feedback comments - Fix for linkage error on Javadocs - Introduced BigtableBatchingCallSettings which will wrap the underlying gax's BatchingCallSettings. This was done for enabling the bigtable throttling to balance the bigtable clusters performance supposedly for batch jobs while still supporting the live load. - Added null check message for RowMutationEntry - Updated javadoc and access level of getter BigtableBatchingCallSettings * fix for Linkage monitor This is a trial fix, as linkage monitor does not seems to be recreated with local build * Addressing feedback comments mostly for javadoc and variable names * Annotate BatchingCallSettings with `@Nonnull`

rahulKQL requested review from igorbernstein2 and kolea2 August 22, 2019 11:24

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 22, 2019

rahulKQL changed the title ~~Introducing new Batching API for Bigtable~~Bigtable: Introducing new Batching API for BigtableAug 22, 2019

igorbernstein2 reviewed Aug 22, 2019
View reviewed changes

Comment thread...-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/RowMutationEntry.java Outdated

igorbernstein2 added the api: bigtable Issues related to the Bigtable API. label Aug 23, 2019

igorbernstein2 requested changes Aug 27, 2019
View reviewed changes

rahulKQL added 2 commits August 27, 2019 16:57

rahulKQL force-pushed the batchingImpl2 branch from bc1a788 to 3f92b34Compare August 27, 2019 12:11

fix for Linkage monitor
754d036
This is a trial fix, as linkage monitor does not seems to be recreated with local build

kolea2 reviewed Aug 27, 2019
View reviewed changes

Comment thread...-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/RowMutationEntry.java

igorbernstein2 requested changes Aug 27, 2019
View reviewed changes