Re: Review Request 25309: Fix output formatting error in job status.

2014-09-04 Thread Mark Chu-Carroll
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25309/ --- (Updated Sept. 4, 2014, 9:24 a.m.) Review request for Aurora, David McLaughlin

Re: Review Request 25309: Fix output formatting error in job status.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25309/#review52308 --- src/main/python/apache/aurora/client/cli/jobs.py

Re: Review Request 25309: Fix output formatting error in job status.

2014-09-04 Thread Mark Chu-Carroll
On Sept. 4, 2014, 11:42 a.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/jobs.py, line 543 https://reviews.apache.org/r/25309/diff/1-2/?file=675766#file675766line543 Curious, why not using multiple '\t' instead of spacing? Looking at the output with standard

Re: Review Request 25309: Fix output formatting error in job status.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25309/#review52311 --- Ship it! Ship It! - Maxim Khutornenko On Sept. 4, 2014, 1:24

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Bill Farner
On Sept. 2, 2014, 4:43 p.m., Bill Farner wrote: I'm concerned that the problem we're solving is underspecified. An immediate issue i have with this patch is that it introduces a memory leak (in practice, this is hidden with default settings due to failover failover hides this).

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Maxim Khutornenko
On Sept. 2, 2014, 4:43 p.m., Bill Farner wrote: I'm concerned that the problem we're solving is underspecified. An immediate issue i have with this patch is that it introduces a memory leak (in practice, this is hidden with default settings due to failover failover hides this).

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52318 --- Ship it! Ship It! - Zameer Manji On Sept. 4, 2014, 3:50 a.m.,

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24915/#review52328 --- Per offline discussion, i've realized my memory leak concern is

Re: Review Request 25285: Upgrade to latest in jetty 7.x series.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25285/ --- (Updated Sept. 4, 2014, 5:48 p.m.) Review request for Aurora, David McLaughlin

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Brian Wickman
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52332 --- Discussed offline but it probably makes sense to not do any status

Review Request 25346: Specify required vagrant version in Vagrantfile.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25346/ --- Review request for Aurora and Kevin Sweeney. Bugs: AURORA-683

Re: Review Request 25346: Specify required vagrant version in Vagrantfile.

2014-09-04 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25346/#review52334 --- Ship it! thanks! - Joe Smith On Sept. 4, 2014, 11:48 a.m., Bill

Re: Review Request 25346: Specify required vagrant version in Vagrantfile.

2014-09-04 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25346/#review52335 --- Ship it! Ship It! - Kevin Sweeney On Sept. 4, 2014, 11:48 a.m.,

Review Request 25350: Fix build problem.

2014-09-04 Thread Mark Chu-Carroll
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25350/ --- Review request for Aurora and Bill Farner. Bugs: aurora-684

Re: Review Request 25350: Fix build problem.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25350/#review52341 --- Ship it! Can you change the subject of the review to be more

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Bill Farner
On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35 https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35 Is this interface useful? The implementation logic is trivial, and it's

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Maxim Khutornenko
On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35 https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35 Is this interface useful? The implementation logic is trivial, and it's

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Bill Farner
On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35 https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35 Is this interface useful? The implementation logic is trivial, and it's

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Bill Farner
On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 120 https://reviews.apache.org/r/24815/diff/1/?file=662756#file662756line120 At first glance, this is odd since ITaskConfig contains the components of a

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Maxim Khutornenko
On Sept. 2, 2014, 10:36 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskLimitValidator.java, line 35 https://reviews.apache.org/r/24815/diff/1/?file=662758#file662758line35 Is this interface useful? The implementation logic is trivial, and it's

Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/ --- Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs:

Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25357/ --- Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-685

Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.

2014-09-04 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52361 --- Ship it! Ship It! - Zameer Manji On Sept. 4, 2014, 2:19 p.m.,

Re: Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25357/#review52362 --- Ship it! lgtm(y still scheduler-naive eyes).

Re: Review Request 25356: Add a factory to produce OneWayUpdate instances based on job state and update settings.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25356/#review52363 ---

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Maxim Khutornenko
On Sept. 4, 2014, 5:44 p.m., Bill Farner wrote: Per offline discussion, i've realized my memory leak concern is ~moot since we already have the unbounded HostAttributes store. I liked your suggestion of augmenting the way this soft state is used: - use a Map, no expiry - when

Review Request 25361: Update vagrant provisioning and aurorabuild scripts to remove the need to sudo to run build commands in the vagrant image.

2014-09-04 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25361/ --- Review request for Aurora and Bill Farner. Bugs: AURORA-688

Review Request 25365: Update slave url to use the thermos port.

2014-09-04 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25365/ --- Review request for Aurora and Bill Farner. Bugs: AURORA-644

Review Request 25366: Set principal field in FrameworkInfo struct.

2014-09-04 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25366/ --- Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and

Re: Review Request 25366: Set principal field in FrameworkInfo struct.

2014-09-04 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25366/#review52371 --- src/main/java/org/apache/aurora/scheduler/DriverFactory.java

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Bill Farner
On Sept. 4, 2014, 5:44 p.m., Bill Farner wrote: Per offline discussion, i've realized my memory leak concern is ~moot since we already have the unbounded HostAttributes store. I liked your suggestion of augmenting the way this soft state is used: - use a Map, no expiry - when

Re: Review Request 25366: Set principal field in FrameworkInfo struct.

2014-09-04 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25366/ --- (Updated Sept. 4, 2014, 4:36 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 25366: Set principal field in FrameworkInfo struct.

2014-09-04 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25366/#review52377 --- Ship it! Ship It! - Joshua Cohen On Sept. 4, 2014, 11:36 p.m.,

Re: Review Request 24915: Adding initial GC task delay on scheduler restart.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24915/#review52376 --- Ship it! LGTM overall.

Re: Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread David McLaughlin
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25357/#review52379 --- Ship it! Ship It! - David McLaughlin On Sept. 4, 2014, 9:42

Re: Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25357/#review52382 --- src/main/java/org/apache/aurora/scheduler/TaskVars.java

Re: Review Request 24815: Refactoring SchedulerCore final part.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24815/ --- (Updated Sept. 5, 2014, 12:09 a.m.) Review request for Aurora and Bill Farner.

Re: Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25357/ --- (Updated Sept. 5, 2014, 12:23 a.m.) Review request for Aurora, David

Re: Review Request 25357: Adding support for per-job task status metrics.

2014-09-04 Thread Maxim Khutornenko
On Sept. 4, 2014, 11:59 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 168 https://reviews.apache.org/r/25357/diff/1/?file=679026#file679026line168 To mitigate the risk of OOM in the scheduler's internal TSDB, we should use untracked stats,

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/ --- (Updated Sept. 4, 2014, 5:24 p.m.) Review request for Aurora, David

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52387 --- src/test/python/apache/aurora/executor/test_thermos_executor.py

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/ --- (Updated Sept. 4, 2014, 5:43 p.m.) Review request for Aurora, David

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
On Sept. 4, 2014, 5:39 p.m., Zameer Manji wrote: src/test/python/apache/aurora/executor/test_thermos_executor.py, line 395 https://reviews.apache.org/r/25337/diff/2/?file=679158#file679158line395 I don't think this is supposed to be here. whew, still pass. - Joe

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Brian Wickman
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52389 --- \m/

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52393 --- Ship it! LGTM mod HttpSignaler mocking. - Maxim Khutornenko On

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/ --- (Updated Sept. 4, 2014, 6:20 p.m.) Review request for Aurora, David

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
On Sept. 4, 2014, 5:50 p.m., Brian Wickman wrote: src/main/python/apache/aurora/executor/common/health_checker.py, lines 131-132 https://reviews.apache.org/r/25337/diff/2/?file=679153#file679153line131 make _healthy and _reason non-private done On Sept. 4, 2014, 5:50 p.m.,

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread David Robinson
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52395 --- Ship it!

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread David McLaughlin
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/#review52397 --- Ship it! lgtm. - David McLaughlin On Sept. 5, 2014, 1:20 a.m.,

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25337/ --- (Updated Sept. 4, 2014, 8 p.m.) Review request for Aurora, David McLaughlin,

Re: Review Request 25337: Preserve executor HealthCheckerThread name

2014-09-04 Thread Joe Smith
On Sept. 4, 2014, 6:23 p.m., David Robinson wrote: src/test/python/apache/aurora/executor/common/test_health_checker.py, line 60 https://reviews.apache.org/r/25337/diff/3/?file=679164#file679164line60 Why do you need to cast num_calls? leftover from the previous test (I