Skip to content

feat: dynamic flow control part 1 #620

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.

Sign up for GitHub

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

Jump to bottom
Merged
igorbernstein2 merged 13 commits into from
Mar 4, 2021
Merged

feat: dynamic flow control part 1 #620

igorbernstein2 merged 13 commits into from
Mar 4, 2021

Conversation

@mutianf
Copy link
Contributor

@mutianf mutianf commented Feb 8, 2021

Implementation of go/veneer-dynamic-flow-control.

Part 1, add the settings for enabling / disabling dynamic flow control. The settings will later be used in EnhancedBigtableStub, see #612 for reference.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@mutianf mutianf requested a review from a team as a code owner February 8, 2021 18:12
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Feb 8, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2021
@mutianf mutianf added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed cla: yes This human has signed the Contributor License Agreement. labels Feb 8, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2021
@codecov
Copy link

codecov bot commented Feb 8, 2021 •
edited
Loading

Codecov Report

Merging #620 (7515f42) into dynamic_flow_control (f0e5d56) will decrease coverage by 22.51%.
The diff coverage is n/a.

Impacted file tree graph

@@                     Coverage Diff                     @@
##             dynamic_flow_control     #620       +/-   ##
===========================================================
- Coverage                   81.71%   59.20%   -22.52%     
+ Complexity                   1156       19     -1137     
===========================================================
  Files                         110        2      -108     
  Lines                        7214      125     -7089     
  Branches                      377       18      -359     
===========================================================
- Hits                         5895       74     -5821     
+ Misses                       1119       34     -1085     
+ Partials                      200       17      -183
Impacted Files Coverage Δ Complexity Δ
...om/google/cloud/bigtable/emulator/v2/Emulator.java 56.52% <0.00%> (-3.32%) 14.00% <0.00%> (ø%)
...gle/cloud/bigtable/admin/v2/internal/NameUtil.java
.../google/cloud/bigtable/data/v2/models/Filters.java
...gle/cloud/bigtable/admin/v2/models/AppProfile.java
.../com/google/cloud/bigtable/data/v2/models/Row.java
...e/cloud/bigtable/admin/v2/models/ColumnFamily.java
...gle/cloud/bigtable/data/v2/models/Validations.java
.../bigtable/admin/v2/models/UpdateBackupRequest.java
...igtable/admin/v2/BaseBigtableTableAdminClient.java
...able/gaxx/reframing/IncompleteStreamException.java
... and 95 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 f0e5d56...03d3dd4. Read the comment docs.

igorbernstein2
igorbernstein2 requested changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

I wonder if we your PRs should point to a separate branch until they are usable? I'm concerned about having to cut a release of the client with only the public surface exposed w/o any of plumbing

@mutianf mutianf changed the base branch from master to dynamic_flow_control February 10, 2021 19:49
igorbernstein2
igorbernstein2 requested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

I like the new approach of always upgrading to DynamicFlowControlSettings, but i think we need to do a bit more work on the defaults

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableBatchingCallSettings.java Outdated
private BatchingCallSettings<RowMutationEntry, Void, BulkMutation, Void> batchingCallSettings;
private boolean isLatencyBasedThrottlingEnabled;
private Long targetRpcLatencyMs;
private DynamicFlowControlSettings dynamicFlowControlSettings;
Copy link
Contributor

@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.

please make these final.
Also if you dont mind, please backfill final for batchingCallSettings

@mutianf mutianf force-pushed the dynamic_flow_control_1 branch from 25faa30 to 29b156b Compare February 11, 2021 22:52
@mutianf mutianf force-pushed the dynamic_flow_control_1 branch from 29b156b to ba29be1 Compare February 11, 2021 22:56
igorbernstein2
igorbernstein2 requested changes Feb 16, 2021
View reviewed changes
...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableBatchingCallSettings.java Outdated
* Enables / disables latency based throttling. If enabling the setting, targetRpcLatency needs
* to be set.
*/
public Builder setLatencyBasedThrottling(
Copy link
Contributor

@igorbernstein2 igorbernstein2 Feb 16, 2021

Choose a reason for hiding this comment

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

Here and everywhere else BetaApi for new public surfaces.

Also, can we split this up into enableLatencyBasedThrottling(targetMs) and disableLatencyBasedThrottling()?

Copy link
Contributor Author

@mutianf mutianf Feb 17, 2021

Choose a reason for hiding this comment

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

This class already has BetaApi annotation, will we still need them for each method?

Will split up the settings.

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableBatchingCallSettings.java
* Gets the {@link DynamicFlowControlSettings} that'll be used to set up a {@link
* FlowController} for throttling.
*
* <p>By default, this will allow a maximum of 1000 entries per channel of {@link
Copy link
Contributor

@igorbernstein2 igorbernstein2 Feb 16, 2021

Choose a reason for hiding this comment

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

I'm not sure if the defaults should be set/documented here. This class can be used for both reads and writes. I'm not sure that the 2 should share their defaults. Can we document this on the BigtableDataSettings or StubSettings?

Copy link
Contributor Author

@mutianf mutianf Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought this class is only for BulkMutation? public final class BigtableBatchingCallSettings extends UnaryCallSettings<BulkMutation, Void> and reads use BigtableBulkReadRowsCallSettings.

I put the documentation here because: default DynamicFlowControlSettings depends on the default settings for maxElementCount and maxRequestBytes. And nowhere else is DynamicFlowControlSettings mentioned. So I thought it might be easier to put all the explanations here?

Copy link
Contributor

@igorbernstein2 igorbernstein2 Feb 17, 2021

Choose a reason for hiding this comment

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

naming is hard :(
BigtableBatchingCallSettings sounds generic and I forgot that we created a separate class for reads

@igorbernstein2 igorbernstein2 merged commit 41c948d into googleapis:dynamic_flow_control Mar 4, 2021
@mutianf mutianf deleted the dynamic_flow_control_1 branch March 4, 2021 20:16
mutianf added a commit to mutianf/java-bigtable that referenced this pull request Apr 10, 2021
@mutianf
feat: dynamic flow control part 1 (googleapis#620)
81f13c7 
* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings
mutianf added a commit to mutianf/java-bigtable that referenced this pull request Apr 10, 2021
@mutianf
feat: dynamic flow control part 1 (googleapis#620)
82a45f5 
* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings
mutianf added a commit to mutianf/java-bigtable that referenced this pull request Apr 10, 2021
@mutianf
feat: dynamic flow control part 1 (googleapis#620)
6b06e40 
* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings
mutianf added a commit to mutianf/java-bigtable that referenced this pull request Apr 10, 2021
@mutianf
feat: dynamic flow control part 1 (googleapis#620)
9d69508 
* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings
mutianf added a commit to mutianf/java-bigtable that referenced this pull request Apr 12, 2021
@mutianf
feat: dynamic flow control part 1 (googleapis#620)
7c8e403 
* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings
igorbernstein2 pushed a commit that referenced this pull request Apr 13, 2021
@mutianf
feat: dynamic flow control (#721)
b61c5ce 
* feat: dynamic flow control part 1 (#620)

* feat: dynamic flow control p1, add settings

* update documents

* move flow controller instantiation to stub.create

* fix test

* add some tests

* split out settings to BatchCallSettings

* add getters for target rpc latency

* fix doc

* more changes in doc

* update document and checks on parameters

* add more docs on flow control configs

* fix test

* split up enable/disable settings

* feat: Dynamic flow control p2 (#670)

* feat: dynamic flow control for bulk mutation batcher

* improve documents

* add integration test

* clean up tests and formatting

* add more comments

* moved FlowControlEventStats into FlowController

* updates based on review p1

* updates based on review p2

* updates on review

* update constant

* fix tests

* make row key prefix random

* updates based on review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

Assignees

No one assigned

Labels

api: bigtable Issues related to the googleapis/java-bigtable API. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mutianf @igorbernstein2