-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scale up up to minSize #282
Conversation
final int targetCapacity = Math.max(minSize, | ||
currentState.getNumDesired() - currentInstanceIdsToTerminate.size() + currentToAdd); |
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.
Should we add additional check as a defense-in-depth against negative targetCapacity?
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.
My assumption is we can provide minSize to be negative (?)
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.
minSize should never be negative because of config.jelly
but maybe this doesn't protect against configuration as code?
Perhaps we should have a more explicit check that minsize >= 0 when initializing the cloud and throw an error at that point?
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 not throw error, maybe just log that as an error.
currentState = new FleetStateStats(currentState, targetCapacity); | ||
|
||
updateByState(currentToAdd, currentInstanceIdsToTerminate, targetCapacity, currentState); | ||
FleetStateStats updatedState = updateByState(currentToAdd, currentInstanceIdsToTerminate, currentState); |
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.
Do we need to introduce updatedState
. Once the fleet gets updated at this point, the currentState
obj remains stale. Is there any way you can avoid 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.
not sure I get what you mean, can you elaborate?
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'm trying to understand why introduce updatedState
instead of currentState
.
Sample snippet:
currentState = new FleetStateStats(currentState, targetCapacity);
instead of
final FleetStateStats updatedState = new FleetStateStats(currentState, targetCapacity);
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.
ah I see. I added it because I thought it made it clearer that the state was being changed.
and I changed updateByState
to return FleetStateStats because we were modifying the state inside of that method but it wasn't getting captured
Related, do you know what benefit we get out of constructing a new object instead of having a setter for targetCapacity? Since currentState is a local var it seems like it'd be fine?
i.e. why do we need
currentState = new FleetStateStats(currentState, targetCapacity);
instead of
currentState.setTargetCapacity(targetCapcity);
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 guess we are updating number of active instances too so probably creating new object instead of just updating targetCapacity. We should probably just re-use currentState object to point to the newer state object and let updateByState return FleetStateStats.
5ae5a1c
to
bb87858
Compare
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.
lgtm
Fixes #274