feat: add max/min throttling options to BulkWriterOptions by thebrianchen · Pull Request #400 · googleapis/java-firestore

thebrianchen · 2020-10-02T20:44:10Z

Porting throttling options and rate limiter test fix from node.

schmidt-sebastian · 2020-10-02T22:11:24Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+      // The initial validation step ensures that the maxOpsPerSecond is
+      // greater than initialOpsPerSecond. If this inequality is true, that
+      // means initialOpsPerSecond was not set and maxOpsPerSecond is less
+      // than the default starting rate.


These comments need some reflowing to match the Java line length.

schmidt-sebastian · 2020-10-02T22:12:53Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+
+    if (initialRate < 1) {
+      throw FirestoreException.invalidState(
+          "Value for argument 'initialOpsPerSecond' must be an integer within [1, Infinity], but was: "


Suggested change

"Value for argument 'initialOpsPerSecond' must be an integer within [1, Infinity], but was: "

"Value for argument 'initialOpsPerSecond' must be greater than 1, but was: "

Easier to read, me thinks.

schmidt-sebastian · 2020-10-02T22:13:14Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+    if (maxRate < 1) {
+      throw FirestoreException.invalidState(
+          "Value for argument 'maxOpsPerSecond' must be an integer within [1, Infinity], but was: "
+              + (int) maxRate);


schmidt-sebastian · 2020-10-02T22:14:55Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOptions.java

 final class BulkWriterOptions {
+  static final double DEFAULT_UNSET_VALUE = 1.1;
+  private final boolean throttling;
+  private double initialOpsPerSecond = DEFAULT_UNSET_VALUE;


Can we make these Double and initialize to null?

Java doesn't allow setting Double to null, but I used Double.NaN.

schmidt-sebastian · 2020-10-02T22:15:47Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOptions.java

+   * @return The BulkWriterOptions object.
+   */
+  @Nonnull
+  public static BulkWriterOptions withInitialOpsPerSecondThrottling(int initialOpsPerSecond) {


Suggested change

public static BulkWriterOptions withInitialOpsPerSecondThrottling(int initialOpsPerSecond) {

public static BulkWriterOptions withInitialOpsPerSecond(double initialOpsPerSecond) {

schmidt-sebastian · 2020-10-02T22:18:01Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOptions.java

+   *     The throttler's allowed operations per second does not ramp up past the specified
+   *     operations per second.


This seems to follow from line above.

schmidt-sebastian · 2020-10-02T22:18:30Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOptions.java

+  private double maxOpsPerSecond = DEFAULT_UNSET_VALUE;

-  private final boolean enableThrottling;
+  BulkWriterOptions(boolean enableThrottling) {


Does this need to be package-private now?

N/A w/ AutoValue.

schmidt-sebastian · 2020-10-02T22:20:07Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOptions.java

@@ -20,15 +20,31 @@

 /** Options used to disable request throttling in BulkWriter. */
 final class BulkWriterOptions {


I think it might be time to make this an actual Builder. You can use AutoValue to reduce the amount of code you have to write. See

java-firestore/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

Line 228 in edaa539

abstract static class QueryOptions {

schmidt-sebastian · 2020-10-02T22:20:44Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

  @Nonnull
  BulkWriter bulkWriter() {
-    return new BulkWriter(this, /* enableThrottling= */ true);
+    return new BulkWriter(this, new BulkWriterOptions(true));


Add back /* enableThrottling= */ ?

schmidt-sebastian · 2020-10-02T22:22:10Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

   * @param initialCapacity Initial maximum number of operations per second.
   * @param multiplier Rate by which to increase the capacity.
   * @param multiplierMillis How often the capacity should increase in milliseconds.
+   * @param maximumCapacity Maximum number of allowed operations per second. The number of tokens


Isn't this different from the capacity? From what I can tell, the capacity grows unbounded and the rate is reduced artificially. The implementation is fine, but the name of the setting is a bit misleading.

When the capacity is calculated, it's capped at the maximumCapacity, which I thought is short for "maximum allowed capacity".

When a new request is made, the number of tokens to allocate is done in calculateCapacity, which performs the Math.min() operation. This means that the capacity is bounded by maximumCapacity.

I think in your implementation the total capacity can grow beyond maximumCapacity, but the rate at which tokens get deducted is limited. This implementation is correct, but the value that is passed here is not the maximum capacity, but rather the limit of tokens used at a time. e.g. after 5 seconds without requests the total capacity of the throttler could be 500, even if maximumCapacity is 100.

As stated, this is just a naming nit.

Thanks for elaborating! Renamed to maximumRate.

codecov · 2020-10-07T22:38:34Z

Codecov Report

Merging #400 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #400      +/-   ##
============================================
+ Coverage     72.40%   72.52%   +0.11%     
- Complexity     1043     1112      +69     
============================================
  Files            64       68       +4     
  Lines          5578     5910     +332     
  Branches        689      768      +79     
============================================
+ Hits           4039     4286     +247     
- Misses         1323     1370      +47     
- Partials        216      254      +38

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/google/cloud/firestore/BulkWriter.java	`72.11% <100.00%> (+5.11%)`	`30.00 <1.00> (+8.00)`
.../com/google/cloud/firestore/BulkWriterOptions.java	`100.00% <100.00%> (+100.00%)`	`2.00 <2.00> (+2.00)`
...java/com/google/cloud/firestore/FirestoreImpl.java	`77.61% <100.00%> (+0.74%)`	`27.00 <2.00> (+2.00)`
...n/java/com/google/cloud/firestore/RateLimiter.java	`100.00% <100.00%> (ø)`	`15.00 <4.00> (+2.00)`
...om/google/cloud/firestore/AutoValue_FieldPath.java	`78.26% <0.00%> (ø)`	`6.00% <0.00%> (?%)`
...google/cloud/firestore/AutoValue_ResourcePath.java	`69.56% <0.00%> (ø)`	`7.00% <0.00%> (?%)`
.../cloud/firestore/AutoValue_Query_QueryOptions.java	`72.63% <0.00%> (ø)`	`34.00% <0.00%> (?%)`
...e/cloud/firestore/AutoValue_BulkWriterOptions.java	`46.03% <0.00%> (ø)`	`8.00% <0.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 91d57e0...e9b48f3. Read the comment docs.

schmidt-sebastian

Please replace double with a nullable Double, which allows you to remove your Double.NaN checks. Otherwise, this looks good. Thanks!

schmidt-sebastian · 2020-10-08T23:55:09Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

    } else {
-      rateLimiter = new RateLimiter(Integer.MAX_VALUE, Double.MAX_VALUE, Integer.MAX_VALUE);
+      double startingRate = DEFAULT_STARTING_MAXIMUM_OPS_PER_SECOND;
+      double maxRate = Integer.MAX_VALUE;


This seems strange. Should we use Double.POSITIVE_INFINITY?

schmidt-sebastian · 2020-10-08T23:55:34Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+      double startingRate = DEFAULT_STARTING_MAXIMUM_OPS_PER_SECOND;
+      double maxRate = Integer.MAX_VALUE;
+
+      if (!Double.isNaN(options.getInitialOpsPerSecond())) {


Please use nullable values (Double instead of double) and use a null-check.

Done, but had to modify the AutoValue setters to support passing in int arguments.

schmidt-sebastian · 2020-10-08T23:57:19Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+  private void validateBulkWriterOptions(BulkWriterOptions options) {
+    double initialRate = options.getInitialOpsPerSecond();
+    double maxRate = options.getMaxOpsPerSecond();
+
+    if (initialRate < 1) {
+      throw FirestoreException.invalidState(
+          "Value for argument 'initialOpsPerSecond' must be greater than 1, but was: "
+              + (int) initialRate);
+    }
+
+    if (maxRate < 1) {
+      throw FirestoreException.invalidState(
+          "Value for argument 'maxOpsPerSecond' must be greater than 1, but was: " + (int) maxRate);
+    }
+
+    if (!Double.isNaN(initialRate) && !Double.isNaN(maxRate) && initialRate > maxRate) {
+      throw FirestoreException.invalidState(
+          "'maxOpsPerSecond' cannot be less than 'initialOpsPerSecond'.");
+    }
+
+    if (!options.getThrottlingEnabled() && (!Double.isNaN(initialRate) || !Double.isNaN(maxRate))) {
+      throw FirestoreException.invalidState(
+          "Cannot set 'initialRate' or 'maxRate' when 'throttlingEnabled' is set to false.");
+    }
+  }


Ideally, this validation would happen in the BulkWriterOptions.build() step. This would follow the precedent in other builders.

This might help: https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#validate

schmidt-sebastian · 2020-10-09T00:01:29Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

   * @param initialCapacity Initial maximum number of operations per second.
   * @param multiplier Rate by which to increase the capacity.
   * @param multiplierMillis How often the capacity should increase in milliseconds.
+   * @param maximumCapacity Maximum number of allowed operations per second. The number of tokens


I think in your implementation the total capacity can grow beyond maximumCapacity, but the rate at which tokens get deducted is limited. This implementation is correct, but the value that is passed here is not the maximum capacity, but rather the limit of tokens used at a time. e.g. after 5 seconds without requests the total capacity of the throttler could be 500, even if maximumCapacity is 100.

As stated, this is just a naming nit.

Brian Chen added 2 commits October 2, 2020 15:34

feat: add max/min throttling options to BulkWriterOptions

f5b8e0c

port rate limiter test fix

79e247f

thebrianchen requested a review from a team October 2, 2020 20:44

thebrianchen self-assigned this Oct 2, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2020

thebrianchen requested a review from schmidt-sebastian October 2, 2020 20:50

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 2, 2020

schmidt-sebastian reviewed Oct 2, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 2, 2020

product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Oct 3, 2020

resolve comments and use AutoValue

201594d

thebrianchen requested a review from a team as a code owner October 7, 2020 21:51

change batch size to 25

3658104

thebrianchen requested a review from schmidt-sebastian October 8, 2020 05:00

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 8, 2020

schmidt-sebastian reviewed Oct 9, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 9, 2020

use null values instead of Double.NaN

64d0689

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 9, 2020

schmidt-sebastian approved these changes Oct 9, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 9, 2020

revert batch size to 500

e9b48f3

thebrianchen merged commit 27a9397 into master Oct 9, 2020

thebrianchen deleted the bc/bulk-555 branch October 9, 2020 18:49

	"Value for argument 'initialOpsPerSecond' must be an integer within [1, Infinity], but was: "
	"Value for argument 'initialOpsPerSecond' must be greater than 1, but was: "

	public static BulkWriterOptions withInitialOpsPerSecondThrottling(int initialOpsPerSecond) {
	public static BulkWriterOptions withInitialOpsPerSecond(double initialOpsPerSecond) {

		* The throttler's allowed operations per second does not ramp up past the specified
		* operations per second.

		@@ -20,15 +20,31 @@

		/** Options used to disable request throttling in BulkWriter. */
		final class BulkWriterOptions {

Conversation

thebrianchen commented Oct 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 7, 2020 •

edited

Loading