feat: dynamic flow control part 1 - add FlowController to Batcher #1289

mutianf · 2021-02-03T20:57:15Z

Splitting #1288 into smaller prs.

Implementing go/veneer-dynamic-flow-control. Adding a FlowController to Batcher so in flight requests can be throttled.

codecov · 2021-02-03T20:59:07Z

Codecov Report

Merging #1289 (22213e9) into master (68644a4) will increase coverage by 0.11%.
The diff coverage is 86.20%.

@@             Coverage Diff              @@
##             master    #1289      +/-   ##
============================================
+ Coverage     79.53%   79.65%   +0.11%     
- Complexity     1239     1248       +9     
============================================
  Files           209      209              
  Lines          5434     5461      +27     
  Branches        454      464      +10     
============================================
+ Hits           4322     4350      +28     
+ Misses          933      928       -5     
- Partials        179      183       +4

Impacted Files	Coverage Δ	Complexity Δ
.../java/com/google/api/gax/batching/BatcherImpl.java	`94.94% <84.00%> (-1.84%)`	`22.00 <2.00> (+6.00)`	⬇️
...va/com/google/api/gax/batching/FlowController.java	`86.36% <100.00%> (+8.94%)`	`20.00 <3.00> (+3.00)`

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 68644a4...22213e9. Read the comment docs.

mutianf · 2021-02-05T18:47:00Z

@igorbernstein2 This part should be ready for review :)

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

igorbernstein2 · 2021-02-10T16:57:23Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+              || flowController.getMaxOutstandingRequestBytes()
+                  >= batchingSettings.getRequestByteThreshold(),
+          "if throttling and batching on request bytes are enabled, FlowController#maxOutstandingRequestBytes must be greater or equal to requestByteThreshold");
+    }


Can these checks be moved to FlowControlSettings#build()?

These are the checks to make sure flow control limits are >= batch sizes so it won't deadlock. I also added the checks in BatchingSettings. In case where BatcherImpl is constructed with a FlowController, we'll need to validate the settings here also. (and FlowControlSettings doesn't have any information about batch settings)

ok, makes sense

Please check my comment about construction BatcherImpl directly via FlowController that looks really suspicious, and should be avoided. If we avoid it, we can also get rid of this check. The fact that we need this check here is a one more indicator that flowControlelr should not be passed directly to BatcherImpl.

igorbernstein2 · 2021-02-11T00:37:45Z

LGTM, @vam-google can you take a look and let us know if you are ok with merging this?

vam-google

Most of the comments are minor or questions. My only real concern is the flowController vs new FlowController(batchingSettings.getFlowControlSettings()) thing. Please check the corresponding comment for more details.

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

vam-google · 2021-02-17T00:22:26Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+              || flowController.getMaxOutstandingRequestBytes()
+                  >= batchingSettings.getRequestByteThreshold(),
+          "if throttling and batching on request bytes are enabled, FlowController#maxOutstandingRequestBytes must be greater or equal to requestByteThreshold");
+    }


Please check my comment about construction BatcherImpl directly via FlowController that looks really suspicious, and should be avoided. If we avoid it, we can also get rid of this check. The fact that we need this check here is a one more indicator that flowControlelr should not be passed directly to BatcherImpl.

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

vam-google · 2021-02-17T00:31:45Z

gax/src/main/java/com/google/api/gax/batching/BatchingSettings.java

@@ -174,6 +174,21 @@ public BatchingSettings build() {
          settings.getDelayThreshold() == null
              || settings.getDelayThreshold().compareTo(Duration.ZERO) > 0,
          "delayThreshold must be either unset or positive");
+      if (settings.getFlowControlSettings().getLimitExceededBehavior()


Regardless of how the flowController vs flowControllerSettings issue gets resolved, please make sure that this complex logic exists in only one place (now there are two: here and in BatcherImpl constructor) and reused if needed.

Removed the checks in BatchingSettings and only kept them in BatchImpl. I guess it really depends on the Batcher how these settings are used, so it makes more sense to enforce the checks there?

Sounds reasonable

vam-google · 2021-02-17T00:33:20Z

gax/src/main/java/com/google/api/gax/batching/BatchingSettings.java

+                || settings.getRequestByteThreshold() == null
+                || settings.getFlowControlSettings().getMaxOutstandingRequestBytes()
+                    >= settings.getRequestByteThreshold(),
+            "if throttling and batching on request bytes are enabled, FlowController#maxOutstandingRequestBytes must be greater or equal to requestByteThreshold");


Should those error messages start with capital letter? Also, please split the error sting into mutiple lines, as this one is 160+ characters long and exceeds the formatting limits.

vam-google · 2021-02-17T00:37:36Z

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java

+        new BatcherImpl<>(
+            SQUARER_BATCHING_DESC_V2,
+            callLabeledIntSquarer,
+            labeledIntList,
+            batchingSettings,
+            EXECUTOR)) {


Minor: Looks like this long block is repeated in each test (with an extra flowController parameter sometimes). It uses the try-with-resources block, which leads into heavy multiline formatting. Pleas consider putting the creation of this in the private method, so the try blocks can look something like:

try (BatcherImpl batcher1 = createBatcher(null)) { } // ... try (BatcherImpl batcher2 = createBatcher(flowController)) { }

That will let save quite a few lines of code here and in general make these tests less scary-looking.

vam-google · 2021-02-17T00:40:20Z

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java

+                }
+              });
+      try {
+        future.get(100, TimeUnit.MILLISECONDS);


If TimeoutException is expected, can we make this shorter (i.e. since we are guaranteed to wait, lets wait as little as possible, otherwise it may quickly lead to really long-running unit tests, which should not happen). This comments/question applies to all timed get()s in this test.

vam-google

LGTM

gcf-merge-on-green · 2021-02-19T21:58:02Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

feat: dynamic flow control part 1 - add FlowController to Batcher

74c6322

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 3, 2021

mutianf added 4 commits February 4, 2021 00:06

update test

1961e50

make sure to close batchers in the test

a9bb7a9

Add internal api descriptions

74da1f1

Merge branch 'master' into dynamic_flow_control_p1

cf7f112

mutianf marked this pull request as ready for review February 5, 2021 18:46

mutianf requested review from a team as code owners February 5, 2021 18:46

elharo reviewed Feb 5, 2021

View reviewed changes

mutianf added 2 commits February 5, 2021 20:29

updates based on review

1c10b8b

fix formatting

ca0a2af

elharo reviewed Feb 9, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java Outdated Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java Show resolved Hide resolved

igorbernstein2 reviewed Feb 10, 2021

View reviewed changes

fixing doc

c2971bf

igorbernstein2 requested a review from vam-google February 11, 2021 00:36

vam-google reviewed Feb 17, 2021

View reviewed changes

updates based on code review

666c5e1

vam-google approved these changes Feb 19, 2021

View reviewed changes

Merge branch 'master' into dynamic_flow_control_p1

075a488

igorbernstein2 added the automerge Merge the pull request once unit tests and other checks pass. label Feb 19, 2021

Merge branch 'master' into dynamic_flow_control_p1

22213e9

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 19, 2021

igorbernstein2 merged commit bae5eb6 into googleapis:master Feb 19, 2021

mutianf deleted the dynamic_flow_control_p1 branch February 19, 2021 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynamic flow control part 1 - add FlowController to Batcher #1289

feat: dynamic flow control part 1 - add FlowController to Batcher #1289

mutianf commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading

mutianf commented Feb 5, 2021

igorbernstein2 Feb 10, 2021

mutianf Feb 10, 2021

igorbernstein2 Feb 11, 2021

vam-google Feb 17, 2021

igorbernstein2 commented Feb 11, 2021

vam-google left a comment

vam-google Feb 17, 2021

vam-google Feb 17, 2021

mutianf Feb 18, 2021

vam-google Feb 19, 2021

vam-google Feb 17, 2021

vam-google Feb 17, 2021

vam-google Feb 17, 2021

vam-google left a comment

gcf-merge-on-green bot commented Feb 19, 2021

feat: dynamic flow control part 1 - add FlowController to Batcher #1289

feat: dynamic flow control part 1 - add FlowController to Batcher #1289

Conversation

mutianf commented Feb 3, 2021 • edited Loading

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

mutianf commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorbernstein2 commented Feb 11, 2021

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

gcf-merge-on-green bot commented Feb 19, 2021

mutianf commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading