Introducing interfaces for new Batching API by rahulKQL · Pull Request #692 · googleapis/gax-java

rahulKQL · 2019-03-18T14:04:57Z

What this PR contains

Introducing new Batcher API, which gives an option to perform batching with element object(eg. for bigtable MutateRowsRequest.Entry or for logging LogEntry) directly instead of Request wrappers. This returns a future of result/response(eg. for bigtable MutateRowsResponse.Entry or for logging Void type) corresponding to each element, which would resolve once the batching is finished.

codecov · 2019-03-18T14:08:22Z

Codecov Report

Merging #692 into master will increase coverage by 0.3%.
The diff coverage is 86.92%.

@@             Coverage Diff             @@
##             master     #692     +/-   ##
===========================================
+ Coverage     75.46%   75.77%   +0.3%     
- Complexity     1037     1056     +19     
===========================================
  Files           196      198      +2     
  Lines          4675     4805    +130     
  Branches        363      371      +8     
===========================================
+ Hits           3528     3641    +113     
- Misses          986     1000     +14     
- Partials        161      164      +3

Impacted Files	Coverage Δ	Complexity Δ
.../com/google/api/gax/batching/BatchingSettings.java	`80.95% <ø> (ø)`	`2 <0> (ø)`	⬇️
...va/com/google/api/gax/batching/FlowController.java	`70.58% <0%> (-6.84%)`	`17 <0> (ø)`
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`88.88% <88.88%> (ø)`	`15 <15> (?)`
...gle/api/gax/batching/v2/DefaultBatcherFactory.java	`97.05% <97.05%> (ø)`	`4 <4> (?)`

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 8c9f9a5...671e6f5. Read the comment docs.

rahulKQL · 2019-03-18T14:16:58Z

@igorbernstein2 @sduskis
Please have a look at this change.

igorbernstein2 · 2019-03-18T15:45:38Z

Thanks for working on this!
However I'm having a bit of a hard time following the code. Can you add some docs describing how everything fits together and provide sample usage? Eventually the docs should live in package-info, but while we are working out the initial design it might be better to collaborate in a shared google doc. Please put together a doc with the interfaces of the components and how they interact.

For example:

// Intended usage for MutateRows:
// Bigtable developer will implement a BatchingDescriptor:
class BigtableDescriptor implements BatchingDescriptor  {
// ...
}
// Then they will build a BatcherFactory like so:
// ....

// This factory will be exposed the end user and they will use it to create a batcher like so:
// ..


// Implementation details:
// This is the user visible entrypoint for batching.
class BatcherFactory {
  Batcher createBatcher();
}

// This is the user visible batching context. It will transparently create many batches from the added entries delineated by the thresholds configured in batching settings 
class Batcher<EntryT, EntryResultT> {
  ApiFuture<EntryResultT> add(EntryT entry);
  // ...
  void flush();
  //. ..
}

// This is an internal class that represents an open Batch, it's used to accumulate entries before they are flushed as a batch
class BatchAccumulator {
  // ...
}

General feedback:

It's very hard to understand the separation of responsibility between Batcher/BatcherImpl/Builder, BatchAccumalator and RequestBuilder.
Having a Builder for a BatcherFactory and having Batcher Builder and a BatcherFactory smells weird
Prefixing interface with I doesn't quite fit with the code style of this repo.
How do you pass context to a Batcher? ie, in Bigtable batching we need a target table
Everything in gax-java is intended to be used with the gapic generator, I'm having a

Is this implementation intended for bigtable? If so, I think it would be better to move discussion to bigtable's gaxx package.

codecov-io · 2019-03-29T12:16:01Z

Codecov Report

Merging #692 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #692   +/-   ##
=========================================
  Coverage     75.74%   75.74%           
  Complexity     1041     1041           
=========================================
  Files           196      196           
  Lines          4679     4679           
  Branches        363      363           
=========================================
  Hits           3544     3544           
  Misses          975      975           
  Partials        160      160

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 c975bd9...b1a9066. Read the comment docs.

codecov-io · 2019-03-29T12:16:02Z

Codecov Report

Merging #692 into master will increase coverage by 0.3%.
The diff coverage is 86.92%.

@@             Coverage Diff             @@
##             master     #692     +/-   ##
===========================================
+ Coverage     75.46%   75.77%   +0.3%     
- Complexity     1037     1056     +19     
===========================================
  Files           196      198      +2     
  Lines          4675     4805    +130     
  Branches        363      371      +8     
===========================================
+ Hits           3528     3641    +113     
- Misses          986     1000     +14     
- Partials        161      164      +3

Impacted Files	Coverage Δ	Complexity Δ
.../com/google/api/gax/batching/BatchingSettings.java	`80.95% <ø> (ø)`	`2 <0> (ø)`	⬇️
...va/com/google/api/gax/batching/FlowController.java	`70.58% <0%> (-6.84%)`	`17 <0> (ø)`
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`88.88% <88.88%> (ø)`	`15 <15> (?)`
...gle/api/gax/batching/v2/DefaultBatcherFactory.java	`97.05% <97.05%> (ø)`	`4 <4> (?)`

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 d24940b...e4f0f11. Read the comment docs.

## What this PR contain This contains interfaces required to perform batching with elements(`MutateRowsRequest.Entry` or `LogEntry`) object directly instead of RequestWrappers(`MutateRowsRequest` or `WriteLogEntriesRequest`).

rahulKQL · 2019-05-16T13:36:23Z

@igorbernstein2 please have a look and let me know your thoughts on this.

rahulKQL · 2019-05-16T18:54:17Z

@igorbernstein2 Thanks a lot for detailed review comments. I have addressed them in the last commit.
Please have a fresh look at this PR.

igorbernstein2

LGTM

igorbernstein2 · 2019-05-16T22:08:53Z

@vam-google Do you mind taking a look before I merge this?

vam-google · 2019-05-17T18:40:27Z

@igorbernstein2 Basically LGTM with several comments. My only real concern is the naming of th 4 type params in BatchingDescriptor, the way they are named and put now is very confusing.

Have updated type names to increase user understanding by their name. Address some sample changes according to feedback.

rahulKQL · 2019-05-24T14:19:42Z

@vam-google @igorbernstein2
Please let me know about your views on RequestBuilder#add & types sequence. I would update this PR accordingly.

vam-google

LGTM

removed return statement

rahulKQL · 2019-05-29T08:16:33Z

@igorbernstein2 Please take a fresh review, If looks good then please merge.

igorbernstein2 · 2019-05-29T14:01:58Z

+ * }
+ * }</pre>
+ *
+ * @param <ElementT> The request type to perform batching.


@param <ElementT> the type of each individual element to be batched @param <ElementResultT> the type of the result for each individual element @param <RequestT> the type of the request that will contain the accumulated elements @param <ResponseT> the type of the response will be unpacked into the individual element results

igorbernstein2 · 2019-05-29T14:02:35Z

+ * generated by the gapic-generator.
+ *
+ * @param <ElementT> The request type to perform batching.
+ * @param <RequestT> The request wrapper type which bundles {@link ElementT}.


Please use the javadoc suggestion from above

igorbernstein2

LGTM after the javadoc tweak

rahulKQL · 2019-05-29T14:41:38Z

@igorbernstein2
Updated interfaces with updated Javadoc, Please merge if looks good to you.

igorbernstein · 2019-05-29T15:40:30Z

Just noticed that the ‘@BetaApi’ annotations are missing. Please add them in a follow up pr

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

yoshi-automation added the 🚨 This issue needs some love. label Mar 25, 2019

sduskis added needs work This is a pull request that needs a little love. do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed 🚨 This issue needs some love. labels Mar 25, 2019

rahulKQL force-pushed the new_batching_changes branch from 671e6f5 to e4f0f11 Compare March 29, 2019 12:15

rahulKQL force-pushed the new_batching_changes branch from 6ae98b0 to f126cda Compare April 25, 2019 14:56

rahulKQL added 3 commits May 16, 2019 14:37

Initial Commit for fresh approach

ca76f2d

Batcher Implementation with blocking flush

4766935

Introducing new Batching API interfaces

cd61543

## What this PR contain This contains interfaces required to perform batching with elements(`MutateRowsRequest.Entry` or `LogEntry`) object directly instead of RequestWrappers(`MutateRowsRequest` or `WriteLogEntriesRequest`).

rahulKQL force-pushed the new_batching_changes branch from f126cda to cd61543 Compare May 16, 2019 09:08

reverting batching/BatchingSettings

18de344

rahulKQL changed the title ~~WIP: Fresh Batcher API for batching using single entry object~~ Introducing interfaces for new Batching API May 16, 2019

igorbernstein reviewed May 16, 2019

View reviewed changes

rahulKQL added 2 commits May 17, 2019 00:16

Addressing the javadoc related comments

62f2a9c

Removed @Override annotation from Javadoc code sample

c9c857f

igorbernstein2 approved these changes May 16, 2019

View reviewed changes

igorbernstein2 removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love. labels May 16, 2019

igorbernstein2 requested a review from vam-google May 16, 2019 22:08

yoshi-automation added the 🚨 This issue needs some love. label May 16, 2019

removed access specifiers from sample code and some tiny language edits

ec998ba

vam-google reviewed May 17, 2019

View reviewed changes

Updated generic types name for Batcher related interfaces

1c64871

Have updated type names to increase user understanding by their name. Address some sample changes according to feedback.

vam-google approved these changes May 28, 2019

View reviewed changes

Reverting type names and RequestBuilder#build

e2bbd35

removed return statement

igorbernstein2 reviewed May 29, 2019

View reviewed changes

igorbernstein2 approved these changes May 29, 2019

View reviewed changes

Updated @param for types in batcher interfaces

b1a9066

igorbernstein2 merged commit 90e45b5 into googleapis:master May 29, 2019

This was referenced May 29, 2019

Batcher implementation with Synchronized Batcher#flush(without triggers) #716

Merged

Annotating New Batcher interface with @BetaApi #717

Merged

Conversation

rahulKQL commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR contains

Uh oh!

codecov Bot commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rahulKQL commented Mar 18, 2019

Uh oh!

igorbernstein2 commented Mar 18, 2019

Uh oh!

codecov-io commented Mar 29, 2019 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-io commented Mar 29, 2019

Codecov Report

Uh oh!

rahulKQL commented May 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulKQL commented May 16, 2019

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 commented May 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vam-google commented May 17, 2019

Uh oh!

rahulKQL commented May 24, 2019

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

rahulKQL commented May 29, 2019

Uh oh!

igorbernstein2 May 29, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 May 29, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

rahulKQL commented May 29, 2019

Uh oh!

igorbernstein commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rahulKQL commented Mar 18, 2019 •

edited

Loading

codecov Bot commented Mar 18, 2019 •

edited

Loading

codecov-io commented Mar 29, 2019 •

edited by codecov Bot

Loading