You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2017/12/12 12:20:29 UTC
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
GitHub user sjcorbett opened a pull request:
https://github.com/apache/brooklyn-server/pull/916
DynamicCluster's max size applies to all calls to grow
Previously the limit was ignored by resizeByDelta.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/brooklyn-server cluster-max-delta
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/916.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 #916
----
commit 5125850b4a5be5296f680d9b27d5a3f931be2797
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2017-12-12T12:18:29Z
DynamicCluster's max size applies to all calls to grow
Previously the limit was ignored by resizeByDelta.
----
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r156406908
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,12 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ final int desiredSize = getCurrentSize() + delta;
+ if (maxSize != null && desiredSize > maxSize) {
--- End diff --
I think we need a corresponding check for `MIN_SIZE` in `shrink(int delta)`
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
I'm going to merge this - it's definitely an improvement on what was there before, but my question about resizing up to the maxSize still stands @sjcorbett @nakomis. Interested in your thoughts, as to whether we should change that in a future PR.
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r157902790
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,14 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ if (maxSize != null) {
+ final int desiredSize = getCurrentSize() + delta;
+ if (desiredSize > maxSize) {
+ throw new Resizable.InsufficientCapacityException(
--- End diff --
Should we throw if we'd be willing to grow a bit? e.g. if we're current size 1 and asked to resize by delta of 10, but max size is 10 then should we resize to 10 or should we fail? I think we should only fail if `getCurrentSize() == maxSize`
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r158319056
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,14 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ if (maxSize != null) {
+ final int desiredSize = getCurrentSize() + delta;
+ if (desiredSize > maxSize) {
+ throw new Resizable.InsufficientCapacityException(
--- End diff --
I took a fairly strict interpretation of DynamicCluster's contract. If it's not possible to fulfil a request for a particular cluster size, or *n* new members, in full then I thought it should throw. The consumer should be smarter, expect an `InsufficientCapacityException` and ask for less. In the case of an auto-scaler I'd expect it to make a second attempt to resize.
If the arguments to resize and resizeByDelta are suggestions rather than requirements then best-effort is acceptable.
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
retest this please
Test failure is unrelated - I'll try to fix that in a separate PR:
```
2017-12-20 16:24:31,614 INFO TESTNG FAILED CONFIGURATION: "Surefire test" - @AfterMethod org.apache.brooklyn.location.jclouds.JcloudsMaxConcurrencyStubbedTest.tearDown() finished in 0 ms
org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Error in tearDown of class org.apache.brooklyn.location.jclouds.JcloudsMaxConcurrencyStubbedTest
at org.apache.brooklyn.location.jclouds.AbstractJcloudsLiveTest.tearDown(AbstractJcloudsLiveTest.java:110)
at org.apache.brooklyn.location.jclouds.AbstractJcloudsStubbedUnitTest.tearDown(AbstractJcloudsStubbedUnitTest.java:72)
at org.apache.brooklyn.location.jclouds.JcloudsMaxConcurrencyStubbedTest.tearDown(JcloudsMaxConcurrencyStubbedTest.java:117)
Caused by: java.lang.IllegalArgumentException: Unknown machine SshMachineLocation[173.194.32.123:jenkins@173.194.32.123/173.194.32.123:22(id=fyilkxt1h4)]
at org.apache.brooklyn.location.jclouds.JcloudsLocation.release(JcloudsLocation.java:2150)
at org.apache.brooklyn.location.jclouds.AbstractJcloudsLiveTest.releaseMachine(AbstractJcloudsLiveTest.java:181)
at org.apache.brooklyn.location.jclouds.AbstractJcloudsLiveTest.releaseMachineSafely(AbstractJcloudsLiveTest.java:189)
at org.apache.brooklyn.location.jclouds.AbstractJcloudsLiveTest.tearDown(AbstractJcloudsLiveTest.java:91)
... 33 more
```
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r158266008
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,14 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ if (maxSize != null) {
+ final int desiredSize = getCurrentSize() + delta;
+ if (desiredSize > maxSize) {
+ throw new Resizable.InsufficientCapacityException(
--- End diff --
I think throwing here is the right behaviour @aledsage: we cannot grow more that the maximum size allowed. It also preserves backward compatibility.
However, the idea of growing until we reach the max size, regardless of exceeding it is an interesting idea. Maybe this could be another effector, or some parameter to the `resize` to allow it?
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
@nakomis @sjcorbett @tbouron are you suggesting we change the `AutoScalerPolicy` etc to do that, or that we leave it as-is so the auto-scaler doesn't work well with `maxSize` (unless the user duplicates the config in both the cluster and the policy)?
If it was just the UI/CLI, I'd be fine with leaving as-is. The human who called it would get feedback immediately that it failed, and would then know to try again. My issue is with programmatic usage - we need to do one of: 1) leave as-is so that users of auto-scaler etc need to duplicate the `maxSize` config; 2) change `DynamicCluster` as I'm suggesting; or 3) change the programmatic usage of `resize` (but auto-scaler works with anything that is resizable, whereas `maxSize` is only defined on `DynamicCluster`.
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/916
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r156409964
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,12 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ final int desiredSize = getCurrentSize() + delta;
+ if (maxSize != null && desiredSize > maxSize) {
--- End diff --
Which `MIN_SIZE` are you referring to? You only added the max in https://github.com/apache/brooklyn-server/pull/803.
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
@aledsage I agree with @nakomis and @sjcorbett. I this logic should be implemented in the client (UI/CLI) to deal with a `InsufficientCapacityException` by proposing to resize to the max capacity rather than the desired one.
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r156407339
--- Diff: core/src/test/java/org/apache/brooklyn/entity/group/DynamicClusterTest.java ---
@@ -1398,14 +1398,18 @@ public void testChildCommandPermitNotReleasedWhenMemberStartTaskCancelledBeforeS
public void testClusterMaxSize() {
--- End diff --
If we're changing the `shrink` behaviour (see previous comment), then a corresponding `testClusterMinSize` should also be added
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
retest this please
---
[GitHub] brooklyn-server issue #916: DynamicCluster's max size applies to all calls t...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the issue:
https://github.com/apache/brooklyn-server/pull/916
@aledsage regarding resizing up to `maxSize`, I'd favour @sjcorbett's approach of throwing the `InsufficientCapacityException` and leaving it up to the consumer to re-try. Requesting a new size of X, and getting a new size of Y just feels a bit too much like hidden magic
---
[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/916#discussion_r158298780
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -799,6 +795,14 @@ protected Entity replaceMember(Entity member, @Nullable Location memberLoc, Map<
/** <strong>Note</strong> for sub-classes; this method can be called while synchronized on {@link #mutex}. */
protected Collection<Entity> grow(int delta) {
Preconditions.checkArgument(delta > 0, "Must call grow with positive delta.");
+ Integer maxSize = config().get(MAX_SIZE);
+ if (maxSize != null) {
+ final int desiredSize = getCurrentSize() + delta;
+ if (desiredSize > maxSize) {
+ throw new Resizable.InsufficientCapacityException(
--- End diff --
@tbouron I don't think we want another effector or parameter as we wouldn't get the "desired default behaviour" in our existing usage, e.g. from the auto-scaler policy (assuming we can agree what that desired behaviour is).
The reason I think we should resize it to as big as we're allowed is that I believe that is what will work best with our existing usage. For example, if an auto-scaler thinks we need to go to size 11 but we're only allowed to go to 10, then leaving it at the existing much smaller size seems very wrong. The auto-scaler, as currently written, would never discover that if it asked for 10 then it would get it; the auto-scaler would just keep asking for the bigger value that it thinks is necessary.
I admit it will slightly confuse users if they say "resize to 11" and it only goes to 10, without anything but a warn message in the log. However, at least it's done something; and if they call it again with "11" then they'll get their exception.
---