Skip to content

Bigtable: Introducing new Batching API for Bigtable #6133

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.

Sign up for GitHub

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

Jump to bottom
Merged
kolea2 merged 5 commits into from
Aug 27, 2019
Merged

Bigtable: Introducing new Batching API for Bigtable #6133

kolea2 merged 5 commits into from
Aug 27, 2019

Conversation

@rahulKQL
Copy link

@rahulKQL rahulKQL commented Aug 22, 2019

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.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 22, 2019
@rahulKQL rahulKQL changed the title Introducing new Batching API for Bigtable Bigtable: Introducing new Batching API for Bigtable Aug 22, 2019
igorbernstein2
igorbernstein2 reviewed Aug 22, 2019
View reviewed changes
@igorbernstein2
Copy link

igorbernstein2 commented Aug 22, 2019

Can you investigate why the linkage monitor is complaining?

@rahulKQL
Copy link
Author

rahulKQL commented Aug 22, 2019

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
Copy link

codecov bot commented Aug 22, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@             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.

@igorbernstein2 igorbernstein2 added the api: bigtable Issues related to the Bigtable API. label Aug 23, 2019
@rahulKQL
Copy link
Author

rahulKQL commented Aug 27, 2019

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

@rahulKQL
Introducing new Batching API for Bigtable
e0abbf3 
 - 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.
@rahulKQL
Addressing minor feedback comments
3f92b34 
 - 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
@rahulKQL
fix for Linkage monitor
754d036 
This is a trial fix, as linkage monitor does not seems to be recreated with local build
igorbernstein2
igorbernstein2 suggested changes Aug 27, 2019
View reviewed changes
igorbernstein2
igorbernstein2 approved these changes Aug 27, 2019
View reviewed changes
Copy link

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

LGTM

@kolea2
Copy link
Contributor

kolea2 commented Aug 27, 2019 •
edited
Loading

@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
Copy link
Author

rahulKQL commented Aug 27, 2019

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
Copy link

igorbernstein2 commented Aug 27, 2019

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

@elharo
Copy link
Contributor

elharo commented Aug 27, 2019

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.

@suztomo suztomo mentioned this pull request Aug 27, 2019
Closed
@elharo
Copy link
Contributor

elharo commented Aug 27, 2019

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
Copy link

igorbernstein2 commented Aug 27, 2019

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
Copy link

igorbernstein2 commented Aug 27, 2019

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

@suztomo suztomo mentioned this pull request Aug 27, 2019
Closed
@elharo
Copy link
Contributor

elharo commented Aug 27, 2019

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
Copy link
Contributor

kolea2 commented Aug 27, 2019

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.

@kolea2 kolea2 merged commit be9b920 into googleapis:master Aug 27, 2019
@kolea2 kolea2 mentioned this pull request Aug 28, 2019
Merged
jkleckner pushed a commit to jkleckner/seq2bigtable that referenced this pull request Sep 5, 2019
Update to v107.0 bigtable
628bbeb 
Note changes to the bulk mutation batcher:
  googleapis/google-cloud-java#6133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

@kolea2 kolea2 kolea2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rahulKQL @igorbernstein2 @kolea2 @elharo @googlebot