[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...

[sjcorbett]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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 pull request #916: DynamicCluster's max size applies to all ...

[asfgit]

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 ...

[aledsage]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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.


---

[GitHub] brooklyn-server pull request #916: DynamicCluster's max size applies to all ...

[tbouron]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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 pull request #916: DynamicCluster's max size applies to all ...

[aledsage]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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 ...

[sjcorbett]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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 pull request #916: DynamicCluster's max size applies to all ...

[nakomis]

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<
 /** Note for sub-classes; this method can be called 
while synchronized on {@link #mutex}. */
 protected Collection 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 pull request #916: DynamicCluster's max size applies to all ...

[nakomis]

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 pull request #916: DynamicCluster's max size applies to all ...

[sjcorbett]

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 
Date:   2017-12-12T12:18:29Z

DynamicCluster's max size applies to all calls to grow

Previously the limit was ignored by resizeByDelta.




---