-
Notifications
You must be signed in to change notification settings - Fork 119
Add bundling flow control #213
Add bundling flow control #213
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add a new checked exception into the signature of FutureCallable.
@@ -0,0 +1,65 @@ | |||
/* | |||
* Copyright 2016, Google Inc. All rights reserved. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the sentiment of not adding more checked exceptions. Maybe RuntimeException with @throw
doc?
outputCollection.addAll(outBundle.getData()); | ||
List<E> data = outBundle.getData(); | ||
outputCollection.addAll(data); | ||
if (flowController != null) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -103,6 +103,12 @@ | |||
/** Returns the Boolean object to indicate if the bundling is enabled. Default to true */ | |||
public abstract Boolean getIsEnabled(); | |||
|
|||
public abstract Boolean getIsFlowControlEnabled(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -155,6 +161,12 @@ public Builder setRequestByteThreshold(Integer requestByteThreshold) { | |||
*/ | |||
public abstract Builder setIsEnabled(Boolean enabled); | |||
|
|||
public abstract Builder setIsFlowControlEnabled(Boolean enabled); | |||
|
|||
public abstract Builder setFailOnFlowControlLimits(Boolean failOnFlowControlLimits); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
package com.google.api.gax.bundling; | ||
|
||
import com.google.api.gax.grpc.FlowController; | ||
import com.google.api.gax.grpc.FlowController.FlowControlException; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL. More work to come before complete.
@@ -0,0 +1,65 @@ | |||
/* | |||
* Copyright 2016, Google Inc. All rights reserved. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
package com.google.api.gax.bundling; | ||
|
||
import com.google.api.gax.grpc.FlowController; | ||
import com.google.api.gax.grpc.FlowController.FlowControlException; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
outputCollection.addAll(outBundle.getData()); | ||
List<E> data = outBundle.getData(); | ||
outputCollection.addAll(data); | ||
if (flowController != null) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -155,6 +161,12 @@ public Builder setRequestByteThreshold(Integer requestByteThreshold) { | |||
*/ | |||
public abstract Builder setIsEnabled(Boolean enabled); | |||
|
|||
public abstract Builder setIsFlowControlEnabled(Boolean enabled); | |||
|
|||
public abstract Builder setFailOnFlowControlLimits(Boolean failOnFlowControlLimits); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -103,6 +103,12 @@ | |||
/** Returns the Boolean object to indicate if the bundling is enabled. Default to true */ | |||
public abstract Boolean getIsEnabled(); | |||
|
|||
public abstract Boolean getIsFlowControlEnabled(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
- Move FlowController into core - Move BundlingSettings in bundling - Address other PR feedback
Do you have a preference for (a) a new FlowControlRuntimeException type? Or (b) use ApiException, with status code UNKNOWN. My initial thought was to use ApiException, but it seems that we are not using ApiException except in cases where it is the remote call that has failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is "you" here? As for me, I think I'd to have the bundling code throw FlowControlRuntimeException
, and limit the usage of ApiException
to the callable classes. I think the question of whether FlowControlRuntimeException
is wrapped in ApiException
when thrown from the callable classes depends on whether users can take specific action to recover from FlowControlRuntimeException
or not.
@@ -103,6 +104,10 @@ | |||
/** Returns the Boolean object to indicate if the bundling is enabled. Default to true */ | |||
public abstract Boolean getIsEnabled(); | |||
|
|||
/** Get the flow control settings to use. */ | |||
@Nullable | |||
public abstract FlowControlSettings getFlowControlSettings(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* Control whether the FlowController will fail with an exception or block the thread when flow | ||
* control limits are exceeded. | ||
*/ | ||
public abstract boolean getShouldFailOnFlowControlLimits(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* Build the ThresholdBundler. | ||
*/ | ||
/** Set the flow controller for the ThresholdBundler. */ | ||
public Builder<E> setFlowControler(BundlingFlowController<E> flowController) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Also address PR feedback
"You" is @garrettjonesgoogle and @pongad Updated to address feedback and add tests and runtime exception, PTAL |
@@ -129,11 +129,14 @@ private Builder() { | |||
* | |||
* @throws FlowControlException | |||
*/ | |||
public void add(E e) throws FlowControlException { | |||
public synchronized void add(E e) throws FlowControlException { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
flowController.reserve(e); | ||
// We need to reserve resources from flowController outside the lock, but we also need to | ||
// prevent concurrent calls to add(E e) from all reserving flowController resources and then | ||
// waiting to acquire the lock, so this method is marked as synchronized. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
return new AutoValue_FlowControlSettings.Builder().setIsEnabled(true); | ||
return new AutoValue_FlowControlSettings.Builder() | ||
.setIsEnabled(true) | ||
.setExceedLimitBehavior(LimitExceededBehavior.ThrowException); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
flowController.reserve(e); | ||
// We need to reserve resources from flowController outside the lock, but we also need to | ||
// prevent concurrent calls to add(E e) from all reserving flowController resources and then | ||
// waiting to acquire the lock, so this method is marked as synchronized. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -129,11 +129,14 @@ private Builder() { | |||
* | |||
* @throws FlowControlException | |||
*/ | |||
public void add(E e) throws FlowControlException { | |||
public synchronized void add(E e) throws FlowControlException { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
return new AutoValue_FlowControlSettings.Builder().setIsEnabled(true); | ||
return new AutoValue_FlowControlSettings.Builder() | ||
.setIsEnabled(true) | ||
.setExceedLimitBehavior(LimitExceededBehavior.ThrowException); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
============================================
+ Coverage 69.9% 70.35% +0.44%
- Complexity 475 484 +9
============================================
Files 66 67 +1
Lines 2479 2540 +61
Branches 262 268 +6
============================================
+ Hits 1733 1787 +54
- Misses 649 655 +6
- Partials 97 98 +1
Continue to review full report at Codecov.
|
currentOpenBundle.start(); | ||
signalBundleIsReady = true; | ||
} | ||
flowController.reserve(e); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -49,12 +49,23 @@ public static FlowControlSettings getDefaultInstance() { | |||
@Nullable | |||
public abstract Integer getMaxOutstandingRequestBytes(); | |||
|
|||
/** Is flow control enabled. Defaults to true. */ | |||
public abstract boolean getIsEnabled(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@Nullable private final Semaphore outstandingElementCount; | ||
@Nullable private final Semaphore outstandingByteCount; | ||
private final boolean flowControlEnabled; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
return new AutoValue_FlowControlSettings.Builder(); | ||
return new AutoValue_FlowControlSettings.Builder() | ||
.setIsEnabled(true) | ||
.setLimitExceededBehavior(LimitExceededBehavior.ThrowException); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* <p>The expected behavior for each of these values is: ThrowException: the FlowController will | ||
* throw a {@link FlowControlException} if any of the limits are exceeded. Block: the reserve() | ||
* method of FlowController will block until the quote is available to be reserved. Ignore: all | ||
* flow control limits will be ignored; the FlowController is disabled. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
new FlowController( | ||
bundlingSettings.getFlowControlSettings() != null | ||
? bundlingSettings.getFlowControlSettings() | ||
: FlowControlSettings.newBuilder().setIsEnabled(false).build()); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
* limits are exceeded. | ||
* <li>Block: the reserve() method of FlowController will block until the quote is available to be | ||
* reserved. | ||
* <li>Ignore: all flow control limits will be ignored; the FlowController is disabled. | ||
*/ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
PTAL |
LGTM - @pongad ? |
This reverts commit ec2e656.
This is WIP to make sure I am on the right track adding FlowController support to bundling.
One question I had is: at what point (if any) do we want to convert a FlowControlException into an ApiException? Note that if we don't do this, we will end up adding a checked exception (FlowControlException) into the signature of FutureCallable.