You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/12/21 15:24:24 UTC
[GitHub] brooklyn-server pull request #923: DynamicCluster: resize to maxSize, before...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/923
DynamicCluster: resize to maxSize, before throwing InsufficientCapacity
Building on PR #916 and the conversation in https://github.com/apache/brooklyn-server/pull/916#discussion_r157902790, this changes the behaviour of resize so that it will grow to the maxSize.
For example, imaging the extreme case where we have `initialSize=1, maxSize=10`, and an auto-scaler policy decides that the cluster should be resized to 11. Instead of throwing an exception, the behaviour now will be that the first call to `resize(11)` will cause us to resize to the max of 10 (logging a warning). A subsequent call to `resize(11)` by the policy will throw the `InsufficientCapacityException`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server pr916-grow-to-maxSize
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/923.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #923
----
commit 72b0bf1e38330806f5168fb613c3ae75c8307077
Author: Aled Sage <al...@...>
Date: 2017-12-21T15:21:22Z
DynamicCluster: resize to maxSize, before throwing InsufficientCapacity
----
---
[GitHub] brooklyn-server issue #923: DynamicCluster: resize to maxSize, before throwi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/923
makes sense to resize as much as we can, but 100% agree re giving more feedback to user.
a highlight might be nice but not sure how to clear it. the right thing feels like being able to report log messages that happen during execution. i think accept the pain for now until we can collect and include log messages for tasks.
---
[GitHub] brooklyn-server issue #923: DynamicCluster: resize to maxSize, before throwi...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/923
@aledsage Code-wise, this looks fine.
However, as I said in https://github.com/apache/brooklyn-server/pull/916#discussion_r158266008, I don't think this behaviour is the right thing to do. It will be hard to report back to the user what really happens. If one has `initialSize=5, maxSize=10` and resize by a delta of 10, how a user can know that it didn't resize to 15 because of the `maxSize` (the task won't tell this info explicitly) ? What highlight would be written if this was triggered by the `AutoScaller` policy?
Fail fast is not as smart as this but at least, it will make sense to the user. That's why I was suggesting an effector's parameter which would override the behaviour to treat bounds as suggestions, not requirements. It would be a conscious decision from a user/policy rather than an arbitrary decision from our side.
---
[GitHub] brooklyn-server pull request #923: DynamicCluster: resize to maxSize, before...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/923
---
[GitHub] brooklyn-server issue #923: DynamicCluster: resize to maxSize, before throwi...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/923
@tbouron The `DynamicCluster.resize(int)` effector returns the new size. In your example of `resize(15)`, it would now resize to 10 and return the value 10. In the case of calling `DynamicCluster.resizeByDelta(...)` it will return the `Collection<Entity>` that was created, so one could look at the size returned.
This is already the way it behaves if the location just has insufficient capacity. For example, if using a bring-your-own-nodes with limited capacity, it will add as many as it can and return the new size (the other members will be created and will fail, either being quarantined or deleted depending on the configuration). The same applies if your cloud has limited capacity (e.g. aws service-limits for the number of instances in a region).
I think that model is easier to program against (or at least it requires fewer changes to all the places that already use `resize()`), compared to requiring extra parameters/configuration.
I think as it stands it is hard to use - e.g. it won't play nicely with our existing auto-scaler policy. I think the change in this PR is the simplest way to make it behave closer to what people/policies desire (i.e. if one says "resize" then resize as much as you can).
---