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: introducing bulk read API through Batcher #99

Merged
merged 7 commits into from Jan 16, 2020

Conversation

rahulKQL
Copy link
Contributor

@rahulKQL rahulKQL commented Nov 22, 2019

Fixes #7

This change introduces BulkReadAPI on BigtableDataClient. This operation
accepts row keys in a batch mode and behind the scene split them based
on configurable values.

@rahulKQL rahulKQL requested a review from igorbernstein2 Nov 22, 2019
@googlebot googlebot added the cla: yes label Nov 22, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 16, 2019

Codecov Report

No coverage uploaded for pull request base (master@7640ade). Click here to learn what that means.
The diff coverage is 93.68%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #99   +/-   ##
=========================================
  Coverage          ?   81.92%           
  Complexity        ?      971           
=========================================
  Files             ?       99           
  Lines             ?     6021           
  Branches          ?      330           
=========================================
  Hits              ?     4933           
  Misses            ?      910           
  Partials          ?      178
Impacted Files Coverage Δ Complexity Δ
...ud/bigtable/data/v2/stub/EnhancedBigtableStub.java 94.4% <0%> (ø) 19 <0> (?)
...data/v2/stub/BigtableBulkReadRowsCallSettings.java 100% <100%> (ø) 5 <5> (?)
...a/v2/stub/readrows/ReadRowsBatchingDescriptor.java 100% <100%> (ø) 8 <8> (?)
...gle/cloud/bigtable/data/v2/BigtableDataClient.java 92.3% <100%> (ø) 35 <3> (?)
...om/google/cloud/bigtable/data/v2/models/Query.java 67.61% <100%> (ø) 26 <1> (?)
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 95.02% <100%> (ø) 20 <2> (?)

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 7640ade...29b59cb. Read the comment docs.

rahulKQL added 2 commits Jan 13, 2020
This change introduces BulkReadAPI on BigtableDataClient. This operation
accepts row keys in a batch mode and behind the scene fetch rows based
on configurable batches.
.setTableName(NameUtil.formatTableName(PROJECT_ID, INSTANCE_ID, TABLE_ID))
.setAppProfileId(APP_PROFILE_ID)
.setRowsLimit(10)
.build());
Copy link
Contributor

@igorbernstein2 igorbernstein2 Jan 14, 2020

Choose a reason for hiding this comment

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

What are you testing here?

Copy link
Contributor Author

@rahulKQL rahulKQL Jan 14, 2020

Choose a reason for hiding this comment

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

Thanks for pointing it out, Have updated this with valid test case.

@rahulKQL rahulKQL marked this pull request as ready for review Jan 14, 2020
@rahulKQL rahulKQL requested a review from igorbernstein2 Jan 14, 2020
@@ -224,10 +234,14 @@ public boolean isRefreshingChannel() {
public static InstantiatingGrpcChannelProvider.Builder defaultGrpcTransportProviderBuilder() {
return BigtableStubSettings.defaultGrpcTransportProviderBuilder()
// TODO: tune channels
Copy link
Contributor

@igorbernstein2 igorbernstein2 Jan 14, 2020

Choose a reason for hiding this comment

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

todo should move to getDefaultChannelPoolSize

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

LGTM
@kolea2 do you want to take a pass as well?

BatchingSettings.newBuilder()
.setIsEnabled(true)
.setElementCountThreshold(maxElementPerBatch)
.setRequestByteThreshold(400L * 1024L)
Copy link
Collaborator

@kolea2 kolea2 Jan 15, 2020

Choose a reason for hiding this comment

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

can we make this a variable?

Copy link
Contributor Author

@rahulKQL rahulKQL Jan 15, 2020

Choose a reason for hiding this comment

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

We could have a variable here. Although as in this format, it is similar to the existing bulkMutateRowsSettings configuration

bulkMutateRowsSettings =
BigtableBatchingCallSettings.newBuilder(new MutateRowsBatchingDescriptor())
.setRetryableCodes(IDEMPOTENT_RETRY_CODES)
.setRetrySettings(MUTATE_ROWS_RETRY_SETTINGS)
.setBatchingSettings(
BatchingSettings.newBuilder()
.setIsEnabled(true)
.setElementCountThreshold(100L)
.setRequestByteThreshold(20L * 1024 * 1024)
.setDelayThreshold(Duration.ofSeconds(1))
.setFlowControlSettings(
FlowControlSettings.newBuilder()
.setLimitExceededBehavior(LimitExceededBehavior.Block)
.setMaxOutstandingRequestBytes(100L * 1024 * 1024)
.setMaxOutstandingElementCount(1_000L)
.build())
.build());

We might need to change that to have common formatting in separate PR

@kolea2
Copy link
Collaborator

@kolea2 kolea2 commented Jan 15, 2020

Added a few small comments but LGTM overall!

@rahulKQL rahulKQL requested a review from kolea2 Jan 15, 2020
kolea2
kolea2 approved these changes Jan 16, 2020
@igorbernstein2 igorbernstein2 merged commit e87179e into googleapis:master Jan 16, 2020
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants