Introducing auto/timed flush for new Batcher. by rahulKQL · Pull Request #744 · googleapis/gax-java

rahulKQL · 2019-06-25T13:35:01Z

This is related to new Batching API and is a followUp PR after #734

This change contains:

DelayThreshold in BatchingSettings without any default delay value.
BatcherImpl now accepts user-provided scheduledExecutor, which auto triggers sendBatch() after a given delay.
Javadoc & test case for the same.

rahulKQL · 2019-06-25T13:37:40Z

@igorbernstein2 Please have a look

codecov · 2019-06-25T13:43:04Z

Codecov Report

Merging #744 into master will increase coverage by 0.27%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             master   #744      +/-   ##
==========================================
+ Coverage     77.72%    78%   +0.27%     
- Complexity     1094   1100       +6     
==========================================
  Files           198    198              
  Lines          4799   4823      +24     
  Branches        377    382       +5     
==========================================
+ Hits           3730   3762      +32     
+ Misses          898    889       -9     
- Partials        171    172       +1

Impacted Files	Coverage Δ	Complexity Δ
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`94.68% <100%> (+1%)`	`12 <0> (+1)`	⬆️
...m/google/api/gax/batching/v2/BatchingSettings.java	`75% <50%> (-5%)`	`2 <0> (ø)`
...rc/main/java/com/google/api/gax/rpc/Callables.java	`67.85% <0%> (+0.51%)`	`12% <0%> (+2%)`	⬆️
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`82.5% <0%> (+3.75%)`	`11% <0%> (+1%)`	⬆️
.../grpc/GrpcServerStreamingRequestParamCallable.java	`80% <0%> (+80%)`	`2% <0%> (+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 9b3357c...9564189. Read the comment docs.

igorbernstein2

This needs a bit of work. Before updating the code, lets talk through how to address the issues.

The biggest issue here is the race condition between the scheduled flush thread and the caller's thread. The scheduled flush thread can interleave with the caller adding an element and the caller triggering a flush. This can lead to data loss.

Another issue is that scheduling the a flush for each element adds unnecessary overhead. I think it might better to track last flush time and have a single recurring task per batcher rather then per element.

codecov-io · 2019-07-02T12:42:43Z

Codecov Report

Merging #744 into master will increase coverage by 0.2%.
The diff coverage is 97.05%.

@@             Coverage Diff             @@
##             master     #744     +/-   ##
===========================================
+ Coverage     77.94%   78.14%   +0.2%     
- Complexity     1099     1101      +2     
===========================================
  Files           198      198             
  Lines          4806     4832     +26     
  Branches        379      383      +4     
===========================================
+ Hits           3746     3776     +30     
+ Misses          889      886      -3     
+ Partials        171      170      -1

Impacted Files	Coverage Δ	Complexity Δ
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`100% <100%> (+6.32%)`	`13 <4> (+2)`	⬆️
...m/google/api/gax/batching/v2/BatchingSettings.java	`75% <50%> (-5%)`	`2 <0> (ø)`

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 5fd82c7...7c6462e. Read the comment docs.

codecov-io · 2019-07-02T12:42:43Z

Codecov Report

Merging #744 into master will increase coverage by 0.27%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             master   #744      +/-   ##
==========================================
+ Coverage     77.72%    78%   +0.27%     
- Complexity     1094   1100       +6     
==========================================
  Files           198    198              
  Lines          4799   4823      +24     
  Branches        377    382       +5     
==========================================
+ Hits           3730   3762      +32     
+ Misses          898    889       -9     
- Partials        171    172       +1

Impacted Files	Coverage Δ	Complexity Δ
...va/com/google/api/gax/batching/v2/BatcherImpl.java	`94.68% <100%> (+1%)`	`12 <0> (+1)`	⬆️
...m/google/api/gax/batching/v2/BatchingSettings.java	`75% <50%> (-5%)`	`2 <0> (ø)`
...rc/main/java/com/google/api/gax/rpc/Callables.java	`67.85% <0%> (+0.51%)`	`12% <0%> (+2%)`	⬆️
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`82.5% <0%> (+3.75%)`	`11% <0%> (+1%)`	⬆️
.../grpc/GrpcServerStreamingRequestParamCallable.java	`80% <0%> (+80%)`	`2% <0%> (+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 9b3357c...d4481b6. Read the comment docs.

igorbernstein2

A few nits, but LGTM overall @vam-google, can you take a pass?

This PR adds DelayThreshold in Batcher without any default delay value. DelayThreshold cannot be less than 1 as 0 or less throws IllegalStateEx. - Accepting user provided executor to run the auto-flush. - Ongoing Runnable will be unscheduled after Batcher is GCed(PushCurrentBatchRunnable). - Added `elementLock` to skip race condition between user thread and gax-thread. - Empty batches are never sent to UnaryCallable - Added java docs for Delay threshold **Test to verify:** - Auto flush and blocking flush. - Elements are not leaking to more than one batch even if concurrent flushing happens. - Empty batches are never sent. - Refactored Mockito based test for Exception in the descriptor. Feedback Updates - Fix test case for blocking flush - Removed testBlockingFlush by dividing it to two individual test case. - Updated Java docs according to feedback

igorbernstein2

This looks good to me

igorbernstein2 · 2019-07-19T23:04:25Z

  @Override
  public void close() throws InterruptedException {
-    isClosed = true;
+    if (isClosed) return;


Please use braces for the body:
https://google.github.io/styleguide/javaguide.html#s4.1.1-braces-always-used

igorbernstein2 · 2019-07-19T23:28:49Z

I'm going to merge this for now to unblock further work.
When @vam-google gets back from vacation we can circle back to this and improve it

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

igorbernstein2 added the needs work This is a pull request that needs a little love. label Jun 25, 2019

igorbernstein2 suggested changes Jun 25, 2019

View reviewed changes

igorbernstein2 suggested changes Jul 1, 2019

View reviewed changes

igorbernstein2 suggested changes Jul 2, 2019

View reviewed changes

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

rahulKQL force-pushed the delay_threshold branch from 5773f90 to e03dd89 Compare July 5, 2019 10:06

igorbernstein suggested changes Jul 5, 2019

View reviewed changes

igorbernstein2 approved these changes Jul 8, 2019

View reviewed changes

rahulKQL force-pushed the delay_threshold branch from 5a8bd6f to c30a6ab Compare July 8, 2019 18:02

igorbernstein2 reviewed Jul 8, 2019

View reviewed changes

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

kolea2 reviewed Jul 10, 2019

View reviewed changes

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java

elharo reviewed Jul 10, 2019

View reviewed changes

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatcherImpl.java Outdated

rahulKQL force-pushed the delay_threshold branch from 475f431 to 7c6462e Compare July 11, 2019 07:19

igorbernstein2 approved these changes Jul 19, 2019

View reviewed changes

igorbernstein2 merged commit 5ad6fee into googleapis:master Jul 19, 2019

rahulKQL mentioned this pull request Jul 26, 2019

FlowContorller for new v2.Batcher #767

Closed

This was referenced Aug 16, 2019

Release gax-java v1.48.0 #776

Closed

Release gax-java v1.48.0 #780

Merged

Conversation

rahulKQL commented Jun 25, 2019

This change contains:

Uh oh!

rahulKQL commented Jun 25, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Jul 2, 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 Jul 2, 2019

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Jun 25, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited by codecov Bot

Loading