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

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

Merged
merged 11 commits into from Feb 19, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Feb 3, 2021

Splitting #1288 into smaller prs.

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

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

codecov bot commented Feb 3, 2021

Codecov Report

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

Impacted file tree graph

@@             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 mutianf marked this pull request as ready for review February 5, 2021 18:46
@mutianf mutianf requested review from a team as code owners February 5, 2021 18:46
@mutianf
Copy link
Contributor Author

mutianf commented Feb 5, 2021

@igorbernstein2 This part should be ready for review :)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

@@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable

|| 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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 719 to 724
new BatcherImpl<>(
SQUARER_BATCHING_DESC_V2,
callLabeledIntSquarer,
labeledIntList,
batchingSettings,
EXECUTOR)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
});
try {
future.get(100, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

@igorbernstein2 igorbernstein2 added the automerge Merge the pull request once unit tests and other checks pass. label Feb 19, 2021
@gcf-merge-on-green
Copy link
Contributor

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.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 19, 2021
@igorbernstein2 igorbernstein2 merged commit bae5eb6 into googleapis:master Feb 19, 2021
@mutianf mutianf deleted the dynamic_flow_control_p1 branch February 19, 2021 23:18
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 25, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.62.0](https://www.github.com/googleapis/gax-java/compare/v1.61.0...v1.62.0) (2021-02-25)


### Features

* deprecate RetrySettings.isJittered [gax-java] ([#1308](https://www.github.com/googleapis/gax-java/issues/1308)) ([68644a4](https://www.github.com/googleapis/gax-java/commit/68644a4e24f29223f8f533a3d353dff7457d9737))
* dynamic flow control part 1 - add FlowController to Batcher ([#1289](https://www.github.com/googleapis/gax-java/issues/1289)) ([bae5eb6](https://www.github.com/googleapis/gax-java/commit/bae5eb6070e690c26b95e7b908d15300aa54ef1c))


### Bug Fixes

* prevent unchecked warnings in gax-httpjson ([#1306](https://www.github.com/googleapis/gax-java/issues/1306)) ([ee370f6](https://www.github.com/googleapis/gax-java/commit/ee370f62c5d411738a9b25cf4cfc095aa06d9e07))
* remove unused @InternalExtensionOnly from CallContext classes ([#1304](https://www.github.com/googleapis/gax-java/issues/1304)) ([a8d3a2d](https://www.github.com/googleapis/gax-java/commit/a8d3a2dca96efdb1ce154a976c3e0844e3f501d6))


### Dependencies

* update google-auth-library to 0.24.0 ([#1315](https://www.github.com/googleapis/gax-java/issues/1315)) ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
* update google-common-protos to 2.0.1 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
* update google-http-client to 1.39.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
* update google-iam ([#1313](https://www.github.com/googleapis/gax-java/issues/1313)) ([327b53c](https://www.github.com/googleapis/gax-java/commit/327b53ca7739d9be6e24305b23af2c7a35cb6f4d))
* update gRPC to 1.36.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
* update opencensus to 0.28.0 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
* update protobuf to 3.15.2 ([772331e](https://www.github.com/googleapis/gax-java/commit/772331eda5c47e9de376e505e7d8ee502b01ec72))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants