> 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:test...@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 > >