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: enhancement for Batcher api to address bulk read #833

Merged
merged 8 commits into from Jan 8, 2020

Conversation

rahulKQL
Copy link
Contributor

@rahulKQL rahulKQL commented Dec 16, 2019

This commit contains enhancements to BatcherAPI.

Currently, there is no ways to know the input user has sent in BatchingDescriptor. There is a good chance that in case of duplicated/non-existing input, the server would only send responses for unique elements.

After this change, the elements sent for batching would be available to the user inside BatchingDescriptor#splitResponse.

@rahulKQL rahulKQL requested review from igorbernstein2 and kolea2 Dec 16, 2019
@googlebot googlebot added the cla: yes label Dec 16, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 16, 2019

Codecov Report

Merging #833 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #833      +/-   ##
============================================
- Coverage     78.73%   78.62%   -0.11%     
- Complexity     1151     1163      +12     
============================================
  Files           202      203       +1     
  Lines          5107     5142      +35     
  Branches        407      413       +6     
============================================
+ Hits           4021     4043      +22     
- Misses          913      925      +12     
- Partials        173      174       +1
Impacted Files Coverage Δ Complexity Δ
...n/java/com/google/api/gax/batching/BatchEntry.java 100% <100%> (ø) 2 <2> (?)
...java/com/google/api/gax/batching/BatcherStats.java 97.4% <100%> (ø) 18 <0> (ø) ⬇️
.../java/com/google/api/gax/batching/BatcherImpl.java 98% <100%> (ø) 16 <0> (ø) ⬇️
...main/java/com/google/api/gax/grpc/ChannelPool.java 45.9% <0%> (-3.04%) 13% <0%> (+2%)
...ain/java/com/google/api/gax/rpc/ClientContext.java 80.88% <0%> (-0.94%) 9% <0%> (+1%)
...gle/api/gax/rpc/InstantiatingWatchdogProvider.java 90.9% <0%> (-0.76%) 16% <0%> (+1%)
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 79.89% <0%> (-0.22%) 35% <0%> (ø)
.../com/google/api/gax/rpc/FixedWatchdogProvider.java 100% <0%> (ø) 10% <0%> (+1%) ⬆️
...src/main/java/com/google/api/gax/rpc/Watchdog.java 80.5% <0%> (+0.91%) 11% <0%> (+5%) ⬆️

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 bbefe87...9f42dc9. Read the comment docs.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

I don't love this approach. You are passing 2 parallel lists to splitResponse: elements and batch and you are updating the surface for 2 classes.

I think it would be cleaner for splitResponse to take a list of Entries, which contain the original element and its corresponding unresolved ApiFuture.

@igorbernstein2 igorbernstein2 added the do not merge label Dec 17, 2019
@rahulKQL
Copy link
Contributor Author

@rahulKQL rahulKQL commented Dec 18, 2019

Thanks a lot for the suggestion.

I have tried to implement the approach you mentioned. Could you please take a look.

@rahulKQL rahulKQL requested a review from igorbernstein2 Dec 18, 2019
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

This is looking good. Can you convert the class to use AutoValue and publish this as a PR?

rahulKQL added 4 commits Dec 30, 2019
This commit contains enhancements to BatcherAPI.

Currently there is no ways to know input user has sent in BatchingDescriptor. There is a better chance that in case of duplicated/non-existenting input server would only sent response for unique elements.

After this change the elements sent for batching would be available to user inside BatchingDescriptor.
…ntry

BatchEntry would contain input element and it's corresponded result future.
@rahulKQL rahulKQL marked this pull request as ready for review Dec 30, 2019
@rahulKQL rahulKQL requested a review from igorbernstein2 Dec 30, 2019
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

This is looking good. @kolea2 can you take a pass as well?

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

LGTM except the nit

@rahulKQL rahulKQL requested a review from kolea2 Jan 7, 2020
@igorbernstein2 igorbernstein2 removed the do not merge label Jan 8, 2020
kolea2
kolea2 approved these changes Jan 8, 2020
@igorbernstein2 igorbernstein2 merged commit 4e4f08b into googleapis:master Jan 8, 2020
3 checks passed
@kolea2 kolea2 mentioned this pull request Jan 9, 2020
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.

None yet

4 participants