You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/09/17 22:40:19 UTC
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 8:40 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-717
https://issues.apache.org/jira/browse/AURORA-717
Repository: aurora
Description
-------
Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
Diff: https://reviews.apache.org/r/25750/diff/
Testing
-------
gradle -Pq build
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53860
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 17, 2014, 9:18 p.m., David McLaughlin wrote:
> > +1 to the code change, UI stuff in particular looks good to me.
> >
> > -1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
>
> Bill Farner wrote:
> Should we change direction a bit and just store the original JobUpdateRequest?
>
> Maxim Khutornenko wrote:
> Storing instance count is completely redundant and prone to integrity problems (i.e. need to make sure instance ranges match instance counts).
>
> Maxim Khutornenko wrote:
> | Should we change direction a bit and just store the original JobUpdateRequest
> Don't really see what it would buy us. All that data is already available in the current schema.
>
> David McLaughlin wrote:
> Instance count is not instanceRanges.length, instead it's an option the user sent as part of the update - the value we were already storing. It's redundant once the instance ranges have been calculated but it is used to make that calculation the first time (in the absence of updateOnlyTheseInstances). I think it's useful for auditing and figuring out why the scheduler made the decision it did.
We already log all thrift requests. This should be enough to restore audit trail when needed:
Sep 17, 2014 3:46:07 PM org.apache.aurora.scheduler.thrift.aop.LoggingInterceptor invoke
INFO: startJobUpdate(JobUpdateRequest(jobKey:JobKey(role:bar_role, environment:devel, name:job_foo), taskConfig:TaskConfig(owner:Identity(role:bar_role, user:foo_user), environment:devel, jobName:job_foo, isService:false, numCpus:1.0, ramMb:8, diskMb:1024, priority:0, maxTaskFailures:1, production:true, constraints:[Constraint(name:host, constraint:<TaskConstraint limit:LimitConstraint(limit:1)>)], requestedPorts:[], taskLinks:{}, contactEmail:testing@twitter.com, executorConfig:ExecutorConfig(name:aurora, data:data)), **instanceCount:6**, settings:JobUpdateSettings(updateGroupSize:0, maxPerInstanceFailures:0, maxFailedInstances:0, maxWaitToInstanceRunningMs:0, minWaitInInstanceRunningMs:0, rollbackOnFailure:false, updateOnlyTheseInstances:null)), foo_user)
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 17, 2014, 9:18 p.m., David McLaughlin wrote:
> > +1 to the code change, UI stuff in particular looks good to me.
> >
> > -1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
>
> Bill Farner wrote:
> Should we change direction a bit and just store the original JobUpdateRequest?
Storing instance count is completely redundant and prone to integrity problems (i.e. need to make sure instance ranges match instance counts).
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 17, 2014, 9:18 p.m., David McLaughlin wrote:
> > +1 to the code change, UI stuff in particular looks good to me.
> >
> > -1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
Should we change direction a bit and just store the original JobUpdateRequest?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 17, 2014, 9:18 p.m., David McLaughlin wrote:
> > +1 to the code change, UI stuff in particular looks good to me.
> >
> > -1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
>
> Bill Farner wrote:
> Should we change direction a bit and just store the original JobUpdateRequest?
>
> Maxim Khutornenko wrote:
> Storing instance count is completely redundant and prone to integrity problems (i.e. need to make sure instance ranges match instance counts).
| Should we change direction a bit and just store the original JobUpdateRequest
Don't really see what it would buy us. All that data is already available in the current schema.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Sept. 17, 2014, 9:18 p.m., David McLaughlin wrote:
> > +1 to the code change, UI stuff in particular looks good to me.
> >
> > -1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
>
> Bill Farner wrote:
> Should we change direction a bit and just store the original JobUpdateRequest?
>
> Maxim Khutornenko wrote:
> Storing instance count is completely redundant and prone to integrity problems (i.e. need to make sure instance ranges match instance counts).
>
> Maxim Khutornenko wrote:
> | Should we change direction a bit and just store the original JobUpdateRequest
> Don't really see what it would buy us. All that data is already available in the current schema.
Instance count is not instanceRanges.length, instead it's an option the user sent as part of the update - the value we were already storing. It's redundant once the instance ranges have been calculated but it is used to make that calculation the first time (in the absence of updateOnlyTheseInstances). I think it's useful for auditing and figuring out why the scheduler made the decision it did.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/#review53746
-----------------------------------------------------------
+1 to the code change, UI stuff in particular looks good to me.
-1 to dropping instanceCount completely though. I thought we mentioned we wanted to capture and store all the original details the user sends? Even if this is purely for auditing and never used internally, I still think it's useful.
- David McLaughlin
On Sept. 17, 2014, 8:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25750/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 8:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-717
> https://issues.apache.org/jira/browse/AURORA-717
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 159b09e7c00175bf3aea893d48cb3953186bd6cb
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 82f2b6d98e8270efb9b6517b1f9782a8c5a9aa39
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
> src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 5d6299f9de6eccd0f1332e11d57dfb910d956011
> src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
> src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
> src/main/thrift/org/apache/aurora/gen/api.thrift 3d0beeaed74aafcec0e24725f443e53a67f6c3a0
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java e09caa63bc0150d7109cb237e80b9efee441dded
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java ca990e73d80e8456e71a97f0bdd0b6f4530d0135
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
>
> Diff: https://reviews.apache.org/r/25750/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25750: Store new task configuration in
JobUpdateConfiguration as InstanceTaskConfig
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25750/
-----------------------------------------------------------
(Updated Sept. 18, 2014, 8:37 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
Rebased.
Bugs: AURORA-717
https://issues.apache.org/jira/browse/AURORA-717
Repository: aurora
Description
-------
Converted newTaskConfig into InstanceTaskConfig to allow multiple instance ranges.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 5ac42b6860a1e99f27b6a4067d370f26943f9212
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java dfaadc8cf6bf1d929e4e5fec8347a804c6478122
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 0737f92bc3ea971c64ca6d84b02da7e2b5b934bf
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java 04a9246467ce140300b3b543bdb98ad4fe8302ff
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d4b8141d9d483a21d18afd9c6fbb2cf639595101
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 25382910704f86e6ca292c7f8eae5990663c4b46
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 33bf7c7e6eeb538cf14fb3b9e58ae058390f7e05
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 029c1051f2a2e19e6040ab633906021ba5eb3cb3
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 0884cc8f0504a953ef694dae0e6b05ba6e2bff61
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js f79acd24db81bceea554b4f92507dd8b083d28bd
src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html 613b5325a3ae53fa61e6bac58bcc6e76950f7031
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 0e7f6ebd35a6dee28a16e28fa7b10b10d20c70c5
src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql a450a090f76fe565924e2f9c5340c10d1f6f05be
src/main/thrift/org/apache/aurora/gen/api.thrift 9f1a28cb747f305c22c7b4fb23798a6da43d930f
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 5a974cf4f4351d925e87797e41e9de5fc9e4d12f
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java b1d79a6855392076e1a31ca7c38ac08ffe9c0317
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 7d4dd3720ed946c1dd10b9f3979ded796fb15d98
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java 1b8e5c2c9e21810589b6770129f742de4f1a67e2
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 0e15a79ae6dc877dfa6ba492efafdf1f89f7d40f
src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java f698b532a3827e59e654d6e07e20f5725aed6768
src/test/resources/org/apache/aurora/gen/api.thrift.md5 c5838761783d85a547688d4f708a75c1fd240201
Diff: https://reviews.apache.org/r/25750/diff/
Testing
-------
gradle -Pq build
./pants src/test/python:all
Thanks,
Maxim Khutornenko