Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53181 --- Ship it! Ship It! - Zameer Manji On Sept. 12, 2014, 7:34 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 7:34 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53182 --- Ship it! src/main/python/apache/aurora/client/cli/task.py https://reviews.apache.org/r/25582/#comment92643 Is this a debug line? - David McLaughlin On Sept. 12, 2014, 2:34 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 2:34 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53184 --- src/test/python/apache/aurora/client/cli/test_task_run.py https://reviews.apache.org/r/25582/#comment92646 test_ssh_job_not_found src/test/python/apache/aurora/client/cli/test_task_run.py https://reviews.apache.org/r/25582/#comment92647 Test the ssh command for proper behavior when no tasks are found within a job or something, I think - Joe Smith On Sept. 12, 2014, 7:34 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 7:34 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
On Sept. 12, 2014, 5:09 p.m., Joe Smith wrote: src/test/python/apache/aurora/client/cli/test_task_run.py, line 228 https://reviews.apache.org/r/25582/diff/1/?file=687672#file687672line228 Test the ssh command for proper behavior when no tasks are found within a job or something, I think I'd go so far as to suggest that docstrings on test methods are probably not necessary. This exemplifies why, they just get copied/pasted from other tests and end up not accurately describing what each test does. I'd vote for descriptive test case names and do away with docstrings entirely. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53184 --- On Sept. 12, 2014, 2:34 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 2:34 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 1:17 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Changes --- Address review comments. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs (updated) - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53189 --- Ship it! Ship It! - Joe Smith On Sept. 12, 2014, 10:17 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 10:17 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25582: Fix error in client task ssh command when the job isn't found.
On Sept. 12, 2014, 10:09 a.m., Joe Smith wrote: src/test/python/apache/aurora/client/cli/test_task_run.py, line 228 https://reviews.apache.org/r/25582/diff/1/?file=687672#file687672line228 Test the ssh command for proper behavior when no tasks are found within a job or something, I think Joshua Cohen wrote: I'd go so far as to suggest that docstrings on test methods are probably not necessary. This exemplifies why, they just get copied/pasted from other tests and end up not accurately describing what each test does. I'd vote for descriptive test case names and do away with docstrings entirely. David McLaughlin wrote: +1 Yeah, that's probably the right move overall, though I'm okay with docstrings for test methods if they give a bit more clarification as opposed to increasing an already-long test-method name. - Joe --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/#review53184 --- On Sept. 12, 2014, 10:17 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25582/ --- (Updated Sept. 12, 2014, 10:17 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: aurora-706 https://issues.apache.org/jira/browse/aurora-706 Repository: aurora Description --- Fix error in client task ssh command when the job isn't found. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/main/python/apache/aurora/client/commands/ssh.py 37a90089b72b86c82466f1819e7881a36bb2f214 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b src/test/python/apache/aurora/client/commands/test_ssh.py 4070b710b005c91fe08dd7906cd93bf3a8cdba9e Diff: https://reviews.apache.org/r/25582/diff/ Testing --- Added new tests to catch this case; Ran all client unit tests, all tests pass. Thanks, Mark Chu-Carroll
Re: Review Request 25529: Add a controller for job updates.
On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 109 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line109 Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String? It seems like a bit of overkill but I dislike the + / in the code. Bill Farner wrote: Maybe a method rather than a class? The / is orthogonal to extracting a helper. I don't care about formatting - just that logs have necessary context. I revisited this and started using IInstanceKey, which is ~exactly what you're describing. Thanks! - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53075 --- On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 11, 2014, 4:53 a.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41 src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42 src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9 src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4 src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531 src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8 src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30 src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
Re: Review Request 25455: Deprecating PopulateJobResult.populated thrift field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25455/ --- (Updated Sept. 12, 2014, 6:35 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Changes --- rebased Repository: aurora Description --- Deprecating PopulateJobResult.populated thrift field. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/main/python/apache/aurora/client/api/updater.py 927b646f32300301f4e0d286465140795faf3d54 src/main/python/apache/aurora/client/cli/jobs.py c302470c76be290d3e715b1f9ac9214d2775924e src/main/python/apache/aurora/client/commands/core.py 1644f84b5887c2f8172789d82de00e6a735f5d0c src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e src/test/python/apache/aurora/client/api/test_updater.py 720885ef1864f325346b7060b9aad2083665a7b1 src/test/python/apache/aurora/client/cli/test_diff.py 38629b63c082cf81cb891dace2a70d9e8f418e18 src/test/python/apache/aurora/client/cli/test_restart.py 92aefe612dd59df75188fd7fc8cf080c9a878dde src/test/python/apache/aurora/client/cli/test_update.py 536d9c79d5a76a966ac175248b0a440b16f2d6b2 src/test/python/apache/aurora/client/commands/test_diff.py aa1e87fd60a8541a9f65f28f718f77d0e1cd5e3a src/test/python/apache/aurora/client/commands/test_restart.py 0b71bbbdbb3058bd89e23d909efc6db1121d76b7 src/test/python/apache/aurora/client/commands/test_update.py 85b8423ab604d44406cb31000f63f8a45885296c src/test/resources/org/apache/aurora/gen/api.thrift.md5 85de4b3e2915f7450ca9f88e2f3a875d9d821885 Diff: https://reviews.apache.org/r/25455/diff/ Testing --- gradle -Pq build ./pants src/test/python:all Thanks, Maxim Khutornenko
Re: Review Request 25259: Add update information to the scheduler UI
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25259/#review53216
---
Mostly just nitpicky style/readability stuff...
src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js
https://reviews.apache.org/r/25259/#comment92717
This might read a little bit cleaner if you chained it all?
return instanceActionLookup[action] || 'UNKNOWN'
.replace(/INSTANCE_/, '')
.replace(/_/g, ' ');
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/25259/#comment92719
I'm unclear on the need to convert these arrays of statuses above to
objects just to check for the presence of a value? Is there a reason we can't
simply use indexOf on the array and save the transform?
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/25259/#comment92723
just inline instanceActionLookup[action] here?
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/25259/#comment92725
given the similarity to the reverse events iteration in
`progressFromEvents` above it might be worthwhile to extract a function like
(but with a better name...):
function reverseEventsFilter(instanceEvents, condition) {
var events = _.sortBy(instanceEvents, 'timestampMs');
var instanceMap = {};
condition = condition || function() {};
for (var i = events.length - 1; i = 0; i++) {
if (instanceMap.hasOwnProperty(events[i].instanceId)
condition(event)) {
instanceMap[event.instanceId] = event;
}
}
return instanceMap;
}
Then the logic here just becomes:
var instanceMap = reverseEventsFilter(details.instanceEvents);
And the logic above in `progressFromEvents` becomes something like:
return Object.keys(reverseEventsFilter(instanceEvents, function(e) {
updateUtil.isInstanceSuccessful(e.action); })).length;
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/25259/#comment92727
kill blank line.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/25259/#comment92726
isSubset checks to make sure subset is truthy, and since it's iterating to
subset.length we can probably skip both of these checks and just make this `if
(inSubset(i))`
Also, how do you feel about passing subset in as a param to isSubset
instead of picking it up via a closure?
- Joshua Cohen
On Sept. 9, 2014, 11:50 p.m., David McLaughlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25259/
---
(Updated Sept. 9, 2014, 11:50 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Bill Farner.
Bugs: AURORA-614
https://issues.apache.org/jira/browse/AURORA-614
Repository: aurora
Description
---
Adds update history to the job page. Adds an update details page.
Diffs
-
src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
de49a1c5497e32ee4db944412e5c87807c742d3c
src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html
c780b0fe98863b5421824a9652a7202da9f4302a
src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css
2a752313cb8ae404605a9458b33237a911eec061
src/main/resources/org/apache/aurora/scheduler/http/ui/job.html
e21dee015897eee62ade8f74e26f042b8e2be734
src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js
fb3b5b12121a6e8a30dbf6fe069557f69a563408
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
3477b7e667459665af9d9dc9d2456793822bc7f7
src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js
7f05a552f3786adb115ff9648f4fadce968230e9
src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js
df2806481fc1c2f263d3afd9b21247e97a18ed57
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
bfd360de45c75441743c8ba24a5c445f4146dba6
src/main/resources/org/apache/aurora/scheduler/http/ui/timeDisplay.html
PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/http/ui/update.html
PRE-CREATION
src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html
PRE-CREATION
src/main/thrift/org/apache/aurora/gen/api.thrift
2b376d663b3bc9264965db58ef89de59b10169ad
Diff: https://reviews.apache.org/r/25259/diff/
Testing
---
./gradlew jsHint
File Attachments
job page
Review Request 25593: Use a ServletContextListener to configure servlets
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- Review request for Aurora, David McLaughlin and Bill Farner. Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25593: Use a ServletContextListener to configure servlets
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/#review53220 --- Ship it! lgtm src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java https://reviews.apache.org/r/25593/#comment92730 Obviously not related, but should these all use Objects.requireNonNull instead of Preconditions.checkNotNull? - Joshua Cohen On Sept. 12, 2014, 9:28 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- (Updated Sept. 12, 2014, 9:28 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25529: Add a controller for job updates.
On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 111 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line111 Instead of embedding the logic into a switch here, would it make sense to make InstanceAction an actual class that encapsulates the logic/defines an interface around performing these actions? A fine suggestion! It resulted in more code, but it's overall cleaner, thanks! On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 113 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line113 use instanceName here? Nuked with switch to IInstanceKey. On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 141 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line141 Should we log (or assert?) here? Are there other actions that we're intentionally not processing, or is it an error if we get an action not covered? Goes away now that switch statement is gone. On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, line 119 https://reviews.apache.org/r/25529/diff/1/?file=685012#file685012line119 Is there a ticket for this? Whoops, that was a reminder for myself, fixed in this diff. Removed TODO. On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 141 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line141 I don't know if the amount of work done by the underlying calls warrants it, but you could DRY this up a bit by having instance vars for `storeProvider.getJobUpdateStore()` (used here, and above) and `update.getSummary()` (used twice below and once above). If you wanted to go completely overboard, `update.getSummary().getJobKey()` is used above and below. They're all accessors. I've done a bit of collapsing, trying for a balance between more code and more DRY. On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 159 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line159 It seems like the logic of what states to transition to from a given state is somewhat spread out over the codebase (c.f. https://reviews.apache.org/r/25300/diff/#)? Is there any way we can centralize that? Thanks, i wasn't happy with this. I've pushed more of this down into JobUpdateStateMachine. On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 300 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line300 Thoughts on changing the Function here to be `FunctionJobUpdateStatus, OptionalJobUpdateStatus` to clean up the null dance below? Sure, done. Downside is that we lose ability to use Functions.forMap(). On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, lines 484-488 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line484 Do we need guards in place in case the updaterStatus here is not ROLLING_BACK or ROLLING_FORWARD (or is it guaranteed to be one of the two? If so, is there danger in the addition of additional statuses in the future rendering that assumption invalid?) I've refactored this to eliminate the fall-through branch. Thanks for the nudge! - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53101 --- On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 11, 2014, 4:53 a.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts
Re: Review Request 25593: Use a ServletContextListener to configure servlets
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- (Updated Sept. 12, 2014, 2:39 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- +jcohen Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25593: Use a ServletContextListener to configure servlets
On Sept. 12, 2014, 2:37 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, lines 369-373 https://reviews.apache.org/r/25593/diff/1/?file=688035#file688035line369 Obviously not related, but should these all use Objects.requireNonNull instead of Preconditions.checkNotNull? done - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/#review53220 --- On Sept. 12, 2014, 2:39 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- (Updated Sept. 12, 2014, 2:39 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25593: Use a ServletContextListener to configure servlets
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- (Updated Sept. 12, 2014, 2:43 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- jcohen's feedback Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25593: Use a ServletContextListener to configure servlets
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/#review53226 --- Ship it! Ship It! - Joshua Cohen On Sept. 12, 2014, 9:43 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25593/ --- (Updated Sept. 12, 2014, 9:43 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Repository: aurora Description --- This paves the way for ShiroWebModule integration. As an added benefit Guice is now instantiating our servlets for us (no injector.getInstance calls in our code). Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 9d8654b4268a13fc9d29d12f8da2c1aa8fda4b2b Diff: https://reviews.apache.org/r/25593/diff/ Testing --- ./gradlew -Pq build No new tests added - the existing JerseyServletModuleTests already cover correctness of wiring. Thanks, Kevin Sweeney
Re: Review Request 25259: Add update information to the scheduler UI
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25259/ --- (Updated Sept. 12, 2014, 10:39 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Bill Farner. Changes --- Forgot to run jshint + forgot to refactor one of the event parsing functions. Bugs: AURORA-614 https://issues.apache.org/jira/browse/AURORA-614 Repository: aurora Description --- Adds update history to the job page. Adds an update details page. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java de49a1c5497e32ee4db944412e5c87807c742d3c src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html c780b0fe98863b5421824a9652a7202da9f4302a src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 2a752313cb8ae404605a9458b33237a911eec061 src/main/resources/org/apache/aurora/scheduler/http/ui/job.html e21dee015897eee62ade8f74e26f042b8e2be734 src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js fb3b5b12121a6e8a30dbf6fe069557f69a563408 src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 3477b7e667459665af9d9dc9d2456793822bc7f7 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 7f05a552f3786adb115ff9648f4fadce968230e9 src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js df2806481fc1c2f263d3afd9b21247e97a18ed57 src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 src/main/resources/org/apache/aurora/scheduler/http/ui/timeDisplay.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/update.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html PRE-CREATION src/main/thrift/org/apache/aurora/gen/api.thrift 2b376d663b3bc9264965db58ef89de59b10169ad Diff: https://reviews.apache.org/r/25259/diff/ Testing --- ./gradlew jsHint File Attachments job page https://reviews.apache.org/media/uploaded/files/2014/09/09/531eca81-a0ba-4438-8bd6-4b50d97b0270__job-progress-small-preview.png update page https://reviews.apache.org/media/uploaded/files/2014/09/09/8e3f2950-7d7e-404e-bca9-6c472b5218f7__update-page-finished.png Thanks, David McLaughlin
Re: Review Request 25529: Add a controller for job updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 12, 2014, 10:54 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs (updated) - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108 src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42 src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4 src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189 src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9 src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4 src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531 src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8 src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30 src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91 src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868 src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91 src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9 src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44 src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
Re: Review Request 25459: Adding quota check into startJobUpdate.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25459/#review53238 --- src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java https://reviews.apache.org/r/25459/#comment92782 You mean inclusive? src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java https://reviews.apache.org/r/25459/#comment92793 This treats the update as though it is purely additive. e.g. if i'm updating my job, i'm charged for 2x that job's resources. I believe the algorithm we'll need to use is max(before_update, after_update) when determining the impact of an update. The trouble here becomes visibility, we need to do some follow-up UI work so users aren't baffled when they're being charged for a paused update. - Bill Farner On Sept. 9, 2014, 12:30 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25459/ --- (Updated Sept. 9, 2014, 12:30 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Wiring TaskLimitValidator into the startJobUpdate API. Diffs - src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 3661f8487985f631e3ea437fe6430e0296376a9e src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java 779e925e4d9e7889e8cfd369cea9a8e5da3554d2 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java 8f18617b2052201f87bb1464314c2ee45b279276 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25459/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
Re: Review Request 25543: Update to pants 0.0.23.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25543/#review53242 --- Ship it! awesome- thanks! - Joe Smith On Sept. 11, 2014, 9:13 a.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25543/ --- (Updated Sept. 11, 2014, 9:13 a.m.) Review request for Aurora, Joe Smith and Brian Wickman. Bugs: aurora-695 https://issues.apache.org/jira/browse/aurora-695 Repository: aurora Description --- - Update build files for the new syntax, which no longer requires 'pants(...)' around target names. - Remove no-longer-supported timeout from python_tests. Diffs - pants 4f9c351888afa1a779415730240093c3dee25dfb src/main/python/apache/aurora/admin/BUILD 7a100d1a4a74aae034082f34db051c9cc31f8540 src/main/python/apache/aurora/client/BUILD bf196bf86b36db0d72f8e096260c9a900f74d07c src/main/python/apache/aurora/client/api/BUILD 70ad38e34f14c6d54b71c8f4b2138f085658110e src/main/python/apache/aurora/client/bin/BUILD 43d747956df0611b0880f64df9955d5f5806901c src/main/python/apache/aurora/client/cli/BUILD ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 src/main/python/apache/aurora/client/commands/BUILD cc16923909a7b26b0f3ac0b47bb37dafdbbf502e src/main/python/apache/aurora/client/hooks/BUILD 9471c4cba5175296030747301e246a65a39aa203 src/main/python/apache/aurora/common/BUILD b879b15127d6691b35880074fd0ceacd866a61ed src/main/python/apache/aurora/common/auth/BUILD 7e96cb2258711b2e2925d18ad9435fa986e86bca src/main/python/apache/aurora/config/BUILD 4f8fad80114ddabac8b25f70bba00119228ec675 src/main/python/apache/aurora/config/schema/BUILD 69d60aebd2a9aa353497406ae578a9997323b07e src/main/python/apache/aurora/executor/BUILD 1ad8f82cdce85cf228c53e088171918e36ed536d src/main/python/apache/aurora/executor/bin/BUILD aeb8aee6f50a0d89714626e933699c0a13b363d9 src/main/python/apache/aurora/executor/common/BUILD 335ebc4809096c5f128846cd846d33910a777968 src/main/python/apache/thermos/BUILD 0dc035f759dd9949997f0c979b3556a350cf8df7 src/main/python/apache/thermos/bin/BUILD 669f9930a3590184dc0f8b5c15c36168e715eb03 src/main/python/apache/thermos/common/BUILD 6015f9e9a23f71bf6dede1f4698fe63dbb4dcfaa src/main/python/apache/thermos/config/BUILD 0531f92ea569ffe36817b645a17fab7a712e5897 src/main/python/apache/thermos/core/BUILD 0d1d339d55ee6a569297614ac734661e5caf7ea4 src/main/python/apache/thermos/monitoring/BUILD 79da0d5cef9436d4a3d83075910decfc93e422a6 src/main/python/apache/thermos/observer/BUILD 49b844ffc1b1d5911fc28d14294d088c3d0b6e4b src/main/python/apache/thermos/observer/bin/BUILD 044ca66b18282daf17a4198ff369d954e14c9b6d src/main/python/apache/thermos/observer/http/BUILD 901ad9c61e4dd1c61f5fbf4467becb8c881a64ed src/main/python/apache/thermos/testing/BUILD dc328a63788381307576b5a43ecdc704bb764473 src/main/thrift/org/apache/aurora/gen/BUILD 947504ec1f9582496952b23e66d7f5f20a168ce7 src/test/python/BUILD f01efff2e4982a475221b5739dfe1e8fd1a41d92 src/test/python/apache/aurora/BUILD 6555b984a713ef786aef5688b206ae9d8017c48d src/test/python/apache/aurora/admin/BUILD 5e170d6c15a95e2511b69e18a255d7364c2e7a4d src/test/python/apache/aurora/client/BUILD 831a72d39b27ca2aca466a38914bbf40ff94 src/test/python/apache/aurora/client/api/BUILD b4f08c6192e6bf6b38665197e98db7235751ae86 src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/commands/BUILD 17933dedfa08c9d12c369087bf801e7c35cdde9b src/test/python/apache/aurora/client/hooks/BUILD f7856a2d5dc7e5d1edc480f64d5db97d88c71b70 src/test/python/apache/aurora/common/BUILD e949ba8859d5567c62623bec9d5ba86a8463fbaa src/test/python/apache/aurora/config/BUILD 37bbd27e13a2a3589faff7285f04e3c44ca57eeb src/test/python/apache/aurora/executor/BUILD 4d43e256ad131223cc1ac36a406d42a979a8a2dd src/test/python/apache/aurora/executor/common/BUILD 7d8934046b56ac2c0c16440cfc571dc370767a14 src/test/python/apache/thermos/BUILD cb93a4622e33ef96855b89a7c138f42033368950 src/test/python/apache/thermos/bin/BUILD 4b59f3879298de9664f168150ea9029e013e7913 src/test/python/apache/thermos/common/BUILD 36fa6a69b5e77a645a65c52fef6ec9343bf541bc src/test/python/apache/thermos/config/BUILD 42445ceccba8dfe8296a22a174aca6123bdfdb52 src/test/python/apache/thermos/core/BUILD 8f5c626c2e89834dbb4938c3c69ef8c79558e12b src/test/python/apache/thermos/monitoring/BUILD ea4005b52be3185e553f7d23fb29b89f68befa50 Diff:
Re: Review Request 25529: Add a controller for job updates.
On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 398 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line398 Spent quite a bit of time reading this method. Would definitely benefit from refactoring. How about at least moving everything down from here to something like maybeEvaluateUpdater()? Bill Farner wrote: I wasn't able to follow your request, can you give some pseudocode to illustrate? Everything down from here deals with processing a MonitorAction acquired on this line and can technically be moved into its own method. That would reduce the recordAndChangeJobUpdateStatus() length and make it a bit more readable (i.e. easier to reason about the recursive way this funciton may be invoked). On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 131 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line131 This value is user-controlled and might be set too low for the kill to succeed. Would it rather make sense to fix it internally (as it is now on the client)? Bill Farner wrote: I think a lower bound when validating is reasonable, but i don't like the idea of changing it silently. A lower bound validation would not quite solve the problem as it still may not be enough to kill a task. I have routinely observed task kills taking longer than the default watch_secs value of 45 seconds. The current client timeout is set to 2.5 minutes of binary backoff countdown. I feel we should follow the same approach on the scheduler and perhaps expose an Arg to make it configurable. On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, lines 123-124 https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line123 This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed. Bill Farner wrote: That's a precondition - this action should not be taken if the instance is not active. Is there anything upstream that ensures the instance is still active though? Is that check part of the same transaction? I am concerned about the race between instance state and this query. Perhaps it's just safer to proceed with instance kill, which will be a no-op in this case. On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 493 https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line493 This seems unconditional. Where does rollbackOnFailure get evaluated? Bill Farner wrote: This changed with a comment above, likely obviating the comment. I meant to point that ROLLING_BACK should be conditional upon the user-specified rollbackOnFailure config value. I could not find where it is honored. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53051 --- On Sept. 12, 2014, 10:54 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/ --- (Updated Sept. 12, 2014, 10:54 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji. Bugs: AURORA-613 https://issues.apache.org/jira/browse/AURORA-613 Repository: aurora Description --- There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain. The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken. As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346 src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078 src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
Re: Review Request 25259: Add update information to the scheduler UI
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25259/#review53244 --- Ship it! src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js https://reviews.apache.org/r/25259/#comment92799 I'd like to see the paused states disambiguated. I could imagine a user resuming an update and being surprised by the direction the update moves. src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js https://reviews.apache.org/r/25259/#comment92818 TODO(davmclau) src/main/resources/org/apache/aurora/scheduler/http/ui/timeDisplay.html https://reviews.apache.org/r/25259/#comment92796 I'd love to see UTC here as well. src/main/resources/org/apache/aurora/scheduler/http/ui/update.html https://reviews.apache.org/r/25259/#comment92798 Is the indenting off here? src/main/resources/org/apache/aurora/scheduler/http/ui/update.html https://reviews.apache.org/r/25259/#comment92797 For my own edification - is there a pattern being followed w.r.t. newlines here? I thought i had the style figured out until here. - Bill Farner On Sept. 12, 2014, 10:39 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25259/ --- (Updated Sept. 12, 2014, 10:39 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Bill Farner. Bugs: AURORA-614 https://issues.apache.org/jira/browse/AURORA-614 Repository: aurora Description --- Adds update history to the job page. Adds an update details page. Diffs - src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java de49a1c5497e32ee4db944412e5c87807c742d3c src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html c780b0fe98863b5421824a9652a7202da9f4302a src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 2a752313cb8ae404605a9458b33237a911eec061 src/main/resources/org/apache/aurora/scheduler/http/ui/job.html e21dee015897eee62ade8f74e26f042b8e2be734 src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js fb3b5b12121a6e8a30dbf6fe069557f69a563408 src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 3477b7e667459665af9d9dc9d2456793822bc7f7 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 7f05a552f3786adb115ff9648f4fadce968230e9 src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js df2806481fc1c2f263d3afd9b21247e97a18ed57 src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6 src/main/resources/org/apache/aurora/scheduler/http/ui/timeDisplay.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/update.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html PRE-CREATION src/main/thrift/org/apache/aurora/gen/api.thrift 2b376d663b3bc9264965db58ef89de59b10169ad Diff: https://reviews.apache.org/r/25259/diff/ Testing --- ./gradlew jsHint File Attachments job page https://reviews.apache.org/media/uploaded/files/2014/09/09/531eca81-a0ba-4438-8bd6-4b50d97b0270__job-progress-small-preview.png update page https://reviews.apache.org/media/uploaded/files/2014/09/09/8e3f2950-7d7e-404e-bca9-6c472b5218f7__update-page-finished.png Thanks, David McLaughlin
Re: Review Request 25459: Adding quota check into startJobUpdate.
On Sept. 12, 2014, 11:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 102 https://reviews.apache.org/r/25459/diff/3/?file=683405#file683405line102 You mean inclusive? Not really. Neither 0 nor max value makes sense in case of update. That, however, does not address the createJob case where max value would be legit. On Sept. 12, 2014, 11:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 110 https://reviews.apache.org/r/25459/diff/3/?file=683405#file683405line110 This treats the update as though it is purely additive. e.g. if i'm updating my job, i'm charged for 2x that job's resources. I believe the algorithm we'll need to use is max(before_update, after_update) when determining the impact of an update. The trouble here becomes visibility, we need to do some follow-up UI work so users aren't baffled when they're being charged for a paused update. Thanks for catching this. It's hard to reason about this logic given 3 different use cases (create, add, update). Need to refactor it a bit deeper to properly address all of them. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25459/#review53238 --- On Sept. 9, 2014, 12:30 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25459/ --- (Updated Sept. 9, 2014, 12:30 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-649 https://issues.apache.org/jira/browse/AURORA-649 Repository: aurora Description --- Wiring TaskLimitValidator into the startJobUpdate API. Diffs - src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 3661f8487985f631e3ea437fe6430e0296376a9e src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java 779e925e4d9e7889e8cfd369cea9a8e5da3554d2 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 src/test/java/org/apache/aurora/scheduler/state/TaskLimitValidatorTest.java 8f18617b2052201f87bb1464314c2ee45b279276 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e Diff: https://reviews.apache.org/r/25459/diff/ Testing --- gradle -Pq build Thanks, Maxim Khutornenko
