You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Jordan Ly <jo...@gmail.com> on 2017/05/30 21:21:13 UTC
Review Request 59640: Prioritize adding instances over updating
instances during an update
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
Bugs: AURORA-1928
https://issues.apache.org/jira/browse/AURORA-1928
Repository: aurora
Description
-------
Currently, when updating a job we choose to update instances naively by ascending instance ID number.
However, it would be better to add new instances before killing and updating older instances.
This patch makes it so the job updater prefers to create new instances, then
update instances, and finally kill instances.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
Diff: https://reviews.apache.org/r/59640/diff/1/
Testing
-------
Ran unit tests and integration tests.
Had to modify some integration tests since we now prefer to create over update -- needed to change
the ordering of actions. Additionally, some unit tests only specified configs for one instance even
though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
when creating. Otherwise, it would try to create instances first even though the test didn't really
care.
Tested different update configurations on the Vagrant cluster: only adding instances, only updating
instances, only killing instances, create & update, update & kill.
Thanks,
Jordan Ly
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176852
-----------------------------------------------------------
This has been merged to master.
- David McLaughlin
On June 1, 2017, 12:02 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 12:02 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> RELEASE-NOTES.md fc77b0268dbb0e0ae2d8783a1a4db40aea40123d
> docs/features/job-updates.md 60968aeb47e787720e3a29b33e31f66d3f0c9839
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/3/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176569
-----------------------------------------------------------
Ship it!
Master (e76862a) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 1, 2017, 12:02 a.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated June 1, 2017, 12:02 a.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> RELEASE-NOTES.md fc77b0268dbb0e0ae2d8783a1a4db40aea40123d
> docs/features/job-updates.md 60968aeb47e787720e3a29b33e31f66d3f0c9839
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/3/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/
-----------------------------------------------------------
(Updated June 1, 2017, 12:02 a.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
Changes
-------
Fix merge conflict
Bugs: AURORA-1928
https://issues.apache.org/jira/browse/AURORA-1928
Repository: aurora
Description
-------
Currently, when updating a job we choose to update instances naively by ascending instance ID number.
However, it would be better to add new instances before killing and updating older instances.
This patch makes it so the job updater prefers to create new instances, then
update instances, and finally kill instances.
Diffs (updated)
-----
RELEASE-NOTES.md fc77b0268dbb0e0ae2d8783a1a4db40aea40123d
docs/features/job-updates.md 60968aeb47e787720e3a29b33e31f66d3f0c9839
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
Diff: https://reviews.apache.org/r/59640/diff/3/
Changes: https://reviews.apache.org/r/59640/diff/2-3/
Testing
-------
Ran unit tests and integration tests.
Had to modify some integration tests since we now prefer to create over update -- needed to change
the ordering of actions. Additionally, some unit tests only specified configs for one instance even
though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
when creating. Otherwise, it would try to create instances first even though the test didn't really
care.
Tested different update configurations on the Vagrant cluster: only adding instances, only updating
instances, only killing instances, create & update, update & kill.
Thanks,
Jordan Ly
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176555
-----------------------------------------------------------
This patch does not apply cleanly against master (e76862a), do you need to rebase?
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 31, 2017, 11:44 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 11:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> RELEASE-NOTES.md 75b3ddb856dc5d889a9006490f57cc58ee7d82fc
> docs/features/job-updates.md 60968aeb47e787720e3a29b33e31f66d3f0c9839
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/2/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/
-----------------------------------------------------------
(Updated May 31, 2017, 11:44 p.m.)
Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
Changes
-------
Responded to comments, added additional test for edge case of creating, updating, and killing at the same time.
Added a release note and modified some documentation. If anyone knows where else I might have to update, please let me know :)
Bugs: AURORA-1928
https://issues.apache.org/jira/browse/AURORA-1928
Repository: aurora
Description
-------
Currently, when updating a job we choose to update instances naively by ascending instance ID number.
However, it would be better to add new instances before killing and updating older instances.
This patch makes it so the job updater prefers to create new instances, then
update instances, and finally kill instances.
Diffs (updated)
-----
RELEASE-NOTES.md 75b3ddb856dc5d889a9006490f57cc58ee7d82fc
docs/features/job-updates.md 60968aeb47e787720e3a29b33e31f66d3f0c9839
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
Diff: https://reviews.apache.org/r/59640/diff/2/
Changes: https://reviews.apache.org/r/59640/diff/1-2/
Testing
-------
Ran unit tests and integration tests.
Had to modify some integration tests since we now prefer to create over update -- needed to change
the ordering of actions. Additionally, some unit tests only specified configs for one instance even
though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
when creating. Otherwise, it would try to create instances first even though the test didn't really
care.
Tested different update configurations on the Vagrant cluster: only adding instances, only updating
instances, only killing instances, create & update, update & kill.
Thanks,
Jordan Ly
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On May 30, 2017, 10:13 p.m., Stephan Erb wrote:
> > Thanks for the patch! That is an interesting idea. I am wondering if it is safe in all cases. For example, if I reduce the resource requirements while increasing the number of instances, my service could potentially breach his assigned quota.
> >
> > The "sliding update" Bill talks about in https://www.youtube.com/watch?v=y1hi7K1lPkk&t=13m25s might be a more robust alternative. It will be more complicated to implement though...
> >
> > I am not working on very large clusters, so I am kind of interested what the others have to say here.
>
> Jordan Ly wrote:
> Thanks for the comment Stephan! You bring up a good point.
>
> From what I understand, your final configuration cannot exceed your production quota no matter what. If you reduce the number of resource requirements while increasing the number of instances, if the overall end resource usage is over the quota your update will be denied.
>
> In the scenario where your end state usage is below your production usage, you may temporarily breach your quota as the update rolls out. For example, if you add new instances and change the resource requirements of all instances, the new instances will be added before the old ones can update. However, at the end of the update, you will be within your quota.
>
> This scenario is already occuring with the current ordering (but with killing instances). Right now, we update in a numerical order meaning killing instances happens last. Let's say you increase the resource requirements but reduce the number of instances: at some point during the update you will temporarily exceed your quota as you have updated old instance with more resources (pushing total utilization over the quota) but have yet to kill other instances (bringing utilization back down to end state within quota).
This is correct. Test coverage for this is here: https://github.com/apache/aurora/blob/master/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java#L593
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176371
-----------------------------------------------------------
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Stephan Erb <se...@apache.org>.
> On May 31, 2017, 12:13 a.m., Stephan Erb wrote:
> > Thanks for the patch! That is an interesting idea. I am wondering if it is safe in all cases. For example, if I reduce the resource requirements while increasing the number of instances, my service could potentially breach his assigned quota.
> >
> > The "sliding update" Bill talks about in https://www.youtube.com/watch?v=y1hi7K1lPkk&t=13m25s might be a more robust alternative. It will be more complicated to implement though...
> >
> > I am not working on very large clusters, so I am kind of interested what the others have to say here.
>
> Jordan Ly wrote:
> Thanks for the comment Stephan! You bring up a good point.
>
> From what I understand, your final configuration cannot exceed your production quota no matter what. If you reduce the number of resource requirements while increasing the number of instances, if the overall end resource usage is over the quota your update will be denied.
>
> In the scenario where your end state usage is below your production usage, you may temporarily breach your quota as the update rolls out. For example, if you add new instances and change the resource requirements of all instances, the new instances will be added before the old ones can update. However, at the end of the update, you will be within your quota.
>
> This scenario is already occuring with the current ordering (but with killing instances). Right now, we update in a numerical order meaning killing instances happens last. Let's say you increase the resource requirements but reduce the number of instances: at some point during the update you will temporarily exceed your quota as you have updated old instance with more resources (pushing total utilization over the quota) but have yet to kill other instances (bringing utilization back down to end state within quota).
>
> David McLaughlin wrote:
> This is correct. Test coverage for this is here: https://github.com/apache/aurora/blob/master/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java#L593
Oh right. Thanks for the clarification!
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176371
-----------------------------------------------------------
On May 30, 2017, 11:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Jordan Ly <jo...@gmail.com>.
> On May 30, 2017, 10:13 p.m., Stephan Erb wrote:
> > Thanks for the patch! That is an interesting idea. I am wondering if it is safe in all cases. For example, if I reduce the resource requirements while increasing the number of instances, my service could potentially breach his assigned quota.
> >
> > The "sliding update" Bill talks about in https://www.youtube.com/watch?v=y1hi7K1lPkk&t=13m25s might be a more robust alternative. It will be more complicated to implement though...
> >
> > I am not working on very large clusters, so I am kind of interested what the others have to say here.
Thanks for the comment Stephan! You bring up a good point.
From what I understand, your final configuration cannot exceed your production quota no matter what. If you reduce the number of resource requirements while increasing the number of instances, if the overall end resource usage is over the quota your update will be denied.
In the scenario where your end state usage is below your production usage, you may temporarily breach your quota as the update rolls out. For example, if you add new instances and change the resource requirements of all instances, the new instances will be added before the old ones can update. However, at the end of the update, you will be within your quota.
This scenario is already occuring with the current ordering (but with killing instances). Right now, we update in a numerical order meaning killing instances happens last. Let's say you increase the resource requirements but reduce the number of instances: at some point during the update you will temporarily exceed your quota as you have updated old instance with more resources (pushing total utilization over the quota) but have yet to kill other instances (bringing utilization back down to end state within quota).
- Jordan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176371
-----------------------------------------------------------
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176371
-----------------------------------------------------------
Thanks for the patch! That is an interesting idea. I am wondering if it is safe in all cases. For example, if I reduce the resource requirements while increasing the number of instances, my service could potentially breach his assigned quota.
The "sliding update" Bill talks about in https://www.youtube.com/watch?v=y1hi7K1lPkk&t=13m25s might be a more robust alternative. It will be more complicated to implement though...
I am not working on very large clusters, so I am kind of interested what the others have to say here.
- Stephan Erb
On May 30, 2017, 11:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176457
-----------------------------------------------------------
Ship it!
lgtm overall. Might want to add one more test case on the ordering for the edge case where we're adding, updating *and* killing instances. E.g. imagine the scenario where a service has 5 instances, but out of band, someone has manually killed instances 0 and 1. Then we come along and we want to update to a new task config, but also reduce the instance count to 4. So, current instances: 2, 3, 4, desired instances: 0, 1, 2, 3. In that scenario I believe we'd add instance 0 and 1, update instance 2 and 3, and kill instance 4.
- Joshua Cohen
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176452
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176367
-----------------------------------------------------------
Ship it!
Master (d7425aa) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Reza Motamedi <re...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176481
-----------------------------------------------------------
Ship it!
+1 to adding the test cases that Josh suggested.
- Reza Motamedi
On May 30, 2017, 9:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 9:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176552
-----------------------------------------------------------
I support this idea.
However, I need to resign from reviewing this diff due to time constraints.
- Zameer Manji
On May 30, 2017, 2:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 2:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176538
-----------------------------------------------------------
Ship it!
Shipt it! Please add a short entry to the RELEASE-NOTES.md as well.
- Stephan Erb
On May 30, 2017, 11:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 59640: Prioritize adding instances over updating
instances during an update
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59640/#review176507
-----------------------------------------------------------
LGTM. Minor comments.
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java
Lines 163 (patched)
<https://reviews.apache.org/r/59640/#comment249890>
private class?
Do we really need this class to be `Serializable`?
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java
Lines 193 (patched)
<https://reviews.apache.org/r/59640/#comment249891>
avoid `this`?
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java
Lines 201-204 (patched)
<https://reviews.apache.org/r/59640/#comment249892>
This seems extra defensive.
- Santhosh Kumar Shanmugham
On May 30, 2017, 2:21 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59640/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 2:21 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb, and Zameer Manji.
>
>
> Bugs: AURORA-1928
> https://issues.apache.org/jira/browse/AURORA-1928
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Currently, when updating a job we choose to update instances naively by ascending instance ID number.
> However, it would be better to add new instances before killing and updating older instances.
>
> This patch makes it so the job updater prefers to create new instances, then
> update instances, and finally kill instances.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 14c2d2de3186271819a5f4e527d3b30fd34d2b21
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 290385d737294e23e9dd50f2631303124aa7af7c
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 611f6b8681c8e0b286cd361bdb5ace1fea39d9a5
>
>
> Diff: https://reviews.apache.org/r/59640/diff/1/
>
>
> Testing
> -------
>
> Ran unit tests and integration tests.
>
> Had to modify some integration tests since we now prefer to create over update -- needed to change
> the ordering of actions. Additionally, some unit tests only specified configs for one instance even
> though desiredInstances is always 2 -- had to make it so the range of configurations is always 0-2
> when creating. Otherwise, it would try to create instances first even though the test didn't really
> care.
>
> Tested different update configurations on the Vagrant cluster: only adding instances, only updating
> instances, only killing instances, create & update, update & kill.
>
>
> Thanks,
>
> Jordan Ly
>
>