Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: dynamic flow control for batcher #1288

Closed
wants to merge 4 commits into from

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Feb 1, 2021

go/veneer-dynamic-flow-control

@mutianf mutianf requested review from a team as code owners February 1, 2021 16:23
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2021
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #1288 (cc662f2) into master (96b92db) will increase coverage by 0.03%.
The diff coverage is 82.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1288      +/-   ##
============================================
+ Coverage     79.23%   79.26%   +0.03%     
- Complexity     1236     1268      +32     
============================================
  Files           209      211       +2     
  Lines          5378     5581     +203     
  Branches        454      517      +63     
============================================
+ Hits           4261     4424     +163     
- Misses          938      951      +13     
- Partials        179      206      +27     
Impacted Files Coverage Δ Complexity Δ
.../google/api/gax/batching/NonBlockingSemaphore.java 76.19% <60.00%> (-5.06%) 6.00 <1.00> (+1.00) ⬇️
.../com/google/api/gax/batching/BatchingSettings.java 78.78% <66.66%> (-6.93%) 2.00 <0.00> (ø)
...e/api/gax/batching/DynamicFlowControlSettings.java 80.00% <80.00%> (ø) 2.00 <2.00> (?)
...va/com/google/api/gax/batching/FlowController.java 79.11% <81.48%> (+1.69%) 32.00 <21.00> (+15.00)
.../java/com/google/api/gax/batching/BatcherImpl.java 95.18% <88.23%> (-2.88%) 25.00 <3.00> (+8.00) ⬇️
...google/api/gax/batching/FlowControlEventStats.java 90.00% <90.00%> (ø) 5.00 <5.00> (?)
...oogle/api/gax/httpjson/ManagedHttpJsonChannel.java 54.00% <100.00%> (ø) 5.00 <1.00> (ø)
...com/google/api/gax/batching/BlockingSemaphore.java 83.33% <100.00%> (+2.38%) 7.00 <1.00> (+1.00)
... and 2 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 96b92db...149aad0. Read the comment docs.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Is there a design doc or an issue for this?

@@ -224,6 +294,16 @@ public void close() throws InterruptedException {
}
}

@InternalApi
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of InternalAPI, this can probably be non-public.


/** Settings for dynamic flow control */
@AutoValue
@InternalApi
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of InternalAPI, this can probably be non-public.

import javax.annotation.Nullable;

/** Record the statistics of flow control events. */
@InternalApi
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of InternalAPI, this can probably be non-public.

}

@InternalApi
public synchronized void increaseThresholds(long elementSteps, long byteSteps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of InternalAPI, this can probably be non-public.

@mutianf
Copy link
Contributor Author

mutianf commented Feb 1, 2021

Is there a design doc or an issue for this?

Yes the design doc is at go/veneer-dynamic-flow-control. The changes are needed for https://github.com/googleapis/java-bigtable to have dynamic flow control.

I've also pinged @igorbernstein2 for a first pass (I don't have access to add reviewers yet), but thank you for taking a look 😃 !

@mutianf mutianf marked this pull request as draft February 1, 2021 17:44
@elharo elharo requested a review from igorbernstein2 February 1, 2021 18:35
@mutianf mutianf force-pushed the dynamic_flow_control branch from 9c7ef74 to 149aad0 Compare February 1, 2021 20:02
@igorbernstein2 igorbernstein2 added do not merge Indicates a pull request not ready for merge, due to either quality or timing. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Feb 3, 2021
@mutianf mutianf closed this Apr 14, 2021
@mutianf mutianf deleted the dynamic_flow_control branch April 14, 2021 15:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants