-
Notifications
You must be signed in to change notification settings - Fork 20
Refactor ModifyQueueCapacityAction to follow builder pattern #365
Conversation
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
=========================================
Coverage ? 67.74%
Complexity ? 2131
=========================================
Files ? 301
Lines ? 13435
Branches ? 1120
=========================================
Hits ? 9101
Misses ? 3930
Partials ? 404 Continue to review full report at Codecov.
|
} | ||
|
||
private void setDefaultBounds(ResourceEnum threadPool) { | ||
// TODO: Move configuration values to rca.conf |
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.
Let's create a task and prioritize this.
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.
Let's create a QueueConfig
class that can extract all queue related configs from RcaConf
and apply defaults/overrides. We don't want to clutter rca.conf
with default values, but also don't want to scatter queue config across multiple places. The queue builder will then just accept a QueueConfig
object and use bounds from it.
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.
As per our discussion, let's track this in a separate issue. Action build should keep a reference of RcaConf class object and read the corresponding action configs(or default configs) from the conf object. The RcaConf object can be either passed in from decider or we can add the RcaConf into AppContext and share it within the process.
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.
issue created #367
//fail to read capacity from node config cache | ||
// return null action | ||
if (currentCapacity == null) { | ||
return null; |
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.
Returning null
here requires callers to handle this explicitly. Instead, we can create an action object that is not actionable
. We should also log an error here as current capacity is expected.
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.
done. Action builder will return a non-actionable default action if it fails to read current capacity from node config cache
} | ||
} | ||
|
||
public Builder setCoolOffPeriod(long coolOffPeriodInMillis) { |
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.
nit: with a Builder, we don't need to prefix all these functions with set...
. We can just call them coolOffPeriod(), increase(), desiredCapacity(), ...
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.
done renamed those method to remove the set prefix
return this; | ||
} | ||
|
||
public Builder increase(boolean isInrease) { |
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.
isIncrease*
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.
thanks, will fix this
private final int lowerBound; | ||
private final int upperBound; |
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.
Why do we want lower and upper bound here? Builder takes these values in consideration for calculating the desired value.
I do not see a place where they are used.
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.
one use case is the decider might use this upper/lower bound to calculate the proper step size for each action. We don't use it at this moment but let's say JVM decider will bucketize the current capacity between the [lower, upper] range and based the bucket this resource is allocated, the decider might choose different step size to tune the resource
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.
we can remove them later if decider is end up not using it
Issue #:
Description of changes:
Refactor ModifyQueueCapacityAction to follow builder pattern. the current constructor in ModifyAction does not allow decider to make changes to some parameters in Action object. So I am creating this PR to add a Builder into the ModifyAction class to expose more parameters for decider to tune
Tests:
n/a
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.