Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 label Feb 3, 2021
@codecov
Copy link

@codecov 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 Feb 5, 2021
@mutianf mutianf requested review from as code owners Feb 5, 2021
@mutianf
Copy link
Contributor Author

@mutianf 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
Collaborator

@igorbernstein2 igorbernstein2 Feb 10, 2021

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

@mutianf mutianf Feb 10, 2021

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
Collaborator

@igorbernstein2 igorbernstein2 Feb 11, 2021

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

@vam-google vam-google Feb 17, 2021

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 igorbernstein2 requested a review from vam-google Feb 11, 2021
@igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Feb 11, 2021

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

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

@vam-google vam-google Feb 17, 2021

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

@vam-google vam-google Feb 17, 2021

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

@mutianf mutianf Feb 18, 2021

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

@vam-google vam-google Feb 19, 2021

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

@vam-google vam-google Feb 17, 2021

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.

new BatcherImpl<>(
SQUARER_BATCHING_DESC_V2,
callLabeledIntSquarer,
labeledIntList,
batchingSettings,
EXECUTOR)) {
Copy link
Contributor

@vam-google vam-google Feb 17, 2021

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

@vam-google vam-google Feb 17, 2021

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

LGTM

@igorbernstein2 igorbernstein2 added the automerge label Feb 19, 2021
@gcf-merge-on-green
Copy link
Contributor

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

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 label Feb 19, 2021
@igorbernstein2 igorbernstein2 merged commit bae5eb6 into googleapis:master Feb 19, 2021
9 checks passed
@mutianf mutianf deleted the dynamic_flow_control_p1 branch Feb 19, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue 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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants