feat: dynamic flow control part 1 by mutianf · Pull Request #620 · googleapis/java-bigtable

mutianf · 2021-02-08T18:11:59Z

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> ☕️

codecov · 2021-02-08T18:26:37Z

Codecov Report

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

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

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

igorbernstein2

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

igorbernstein2 · 2021-02-11T15:33:51Z

  private BatchingCallSettings<RowMutationEntry, Void, BulkMutation, Void> batchingCallSettings;
+  private boolean isLatencyBasedThrottlingEnabled;
+  private Long targetRpcLatencyMs;
+  private DynamicFlowControlSettings dynamicFlowControlSettings;


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

igorbernstein2 · 2021-02-16T16:27:55Z

+     * Enables / disables latency based throttling. If enabling the setting, targetRpcLatency needs
+     * to be set.
+     */
+    public Builder setLatencyBasedThrottling(


Here and everywhere else BetaApi for new public surfaces.

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

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

Will split up the settings.

igorbernstein2 · 2021-02-16T16:30:09Z

+     * 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


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?

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?

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

* 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 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

mutianf added 2 commits February 8, 2021 17:58

feat: dynamic flow control p1, add settings

b7c51c8

update documents

cf2fef5

mutianf requested a review from a team February 8, 2021 18:12

product-auto-label Bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Feb 8, 2021

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

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 Bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 8, 2021

mutianf requested a review from igorbernstein2 February 8, 2021 20:21

igorbernstein2 suggested changes Feb 10, 2021

View reviewed changes

move flow controller instantiation to stub.create

3dda52d

mutianf changed the base branch from master to dynamic_flow_control February 10, 2021 19:49

mutianf added 2 commits February 10, 2021 20:18

fix test

2bb0fe9

add some tests

1f93735

igorbernstein2 suggested changes Feb 10, 2021

View reviewed changes

mutianf added 4 commits February 11, 2021 00:36

split out settings to BatchCallSettings

698b09d

add getters for target rpc latency

f710e7e

fix doc

4524109

more changes in doc

b5c6bd5

igorbernstein2 suggested changes Feb 11, 2021

View reviewed changes

update document and checks on parameters

aa4574b

mutianf force-pushed the dynamic_flow_control_1 branch from 25faa30 to 29b156b Compare February 11, 2021 22:52

add more docs on flow control configs

ba29be1

mutianf force-pushed the dynamic_flow_control_1 branch from 29b156b to ba29be1 Compare February 11, 2021 22:56

fix test

96fea59

igorbernstein2 suggested changes Feb 16, 2021

View reviewed changes

split up enable/disable settings

03d3dd4

igorbernstein2 approved these changes Mar 4, 2021

View reviewed changes

igorbernstein2 merged commit 41c948d into googleapis:dynamic_flow_control Mar 4, 2021

mutianf deleted the dynamic_flow_control_1 branch March 4, 2021 20:16

Uh oh!

Conversation

mutianf commented Feb 8, 2021

Uh oh!

codecov Bot commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

mutianf Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

mutianf Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Feb 8, 2021 •

edited

Loading

mutianf Feb 17, 2021 •

edited

Loading