Re: Review Request 21247: Add config grouping visualisation to job page

[David McLaughlin]

> On May 14, 2014, 4:26 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 386
> > 
> >
> > I see this function call (function definition itself is good) as a 
> > potential land mine. For example, moving this function from line 358 to 
> > 356, would cause buildGroupSummary to break and this error will be very 
> > hard to debug. Your comment here is an indication that this needs special 
> > attention.
> > 
> > Moving this function call into the getTasksForJob(line 398) function 
> > will obviate the need for this comment and will also make the code flow 
> > more straightforward. 
> > 
> > I agree that this controller code can use some refactoring, but I think 
> > this is a potential land mine and there is nothing to be gained from 
> > calling it outside the normal call path.
> > 
> > Please feel free to leave a TODO to refactor getTasksForJob.

buildGroupSummary needs $scope.taskSummary to be defined. If I move it to line 
398, then every reason for this being 'a land mine' still exists. This is 
synchronous, imperative code. Why would it be surprising that you can't just 
move code around without breaking stuff? 

Also, it's not going to be hard to debug. The error will say "Cannot call map 
on undefined" and 

TypeError: Cannot read property 'map' of undefined
at buildGroupSummary (http://localhost:8081/js/controllers.js:372:47)
at new  (http://localhost:8081/js/controllers.js:356:5)
at invoke (http://localhost:8081/js/angular.js:3697:17)
at Object.instantiate (http://localhost:8081/js/angular.js:3708:23)
at http://localhost:8081/js/angular.js:6758:28
at link (http://localhost:8081/js/angular-route.js:906:26)
at nodeLinkFn (http://localhost:8081/js/angular.js:6212:13)
at compositeLinkFn (http://localhost:8081/js/angular.js:5622:15)
at publicLinkFn (http://localhost:8081/js/angular.js:5527:30)
at boundTranscludeFn (http://localhost:8081/js/angular.js:5641:21)  


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42937
---


On May 14, 2014, 1:30 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> ---
> 
> (Updated May 14, 2014, 1:30 a.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Bugs: AURORA-378
> https://issues.apache.org/jira/browse/AURORA-378
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Replace the button to show/hide configs with a bar when there are multiple. 
> Tidied up the display of the configuration data. 
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 983101277ffbd1c4017b29f4c86e61315f1bcc78 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> dc515ac818a9af36522bb07a125cd92ff9230fa2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> cd78c20bee7891a6cdfd19943a6f7e8a9dce33df 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7c80bd769b7e80bef0822846166959925b1bf7f8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> a79276d2486fc1122a8b084ea0614cdae759f88c 
> 
> Diff: https://reviews.apache.org/r/21247/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Job page with only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
> Showing config for only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
> Job page with multiple config groups
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
> Viewing one of the groups from config bar
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
> Viewing multiple configs at the same time
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
> screenshot of really small group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a

Re: Review Request 21247: Add config grouping visualisation to job page

[Suman Karumuri]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42937
---


This looks really good. Thanks for the changes. The only ship it blocker is the 
function call below.


src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js


I see this function call (function definition itself is good) as a 
potential land mine. For example, moving this function from line 358 to 356, 
would cause buildGroupSummary to break and this error will be very hard to 
debug. Your comment here is an indication that this needs special attention.

Moving this function call into the getTasksForJob(line 398) function will 
obviate the need for this comment and will also make the code flow more 
straightforward. 

I agree that this controller code can use some refactoring, but I think 
this is a potential land mine and there is nothing to be gained from calling it 
outside the normal call path.

Please feel free to leave a TODO to refactor getTasksForJob.


- Suman Karumuri


On May 14, 2014, 1:30 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> ---
> 
> (Updated May 14, 2014, 1:30 a.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Bugs: AURORA-378
> https://issues.apache.org/jira/browse/AURORA-378
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Replace the button to show/hide configs with a bar when there are multiple. 
> Tidied up the display of the configuration data. 
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 983101277ffbd1c4017b29f4c86e61315f1bcc78 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> dc515ac818a9af36522bb07a125cd92ff9230fa2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> cd78c20bee7891a6cdfd19943a6f7e8a9dce33df 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7c80bd769b7e80bef0822846166959925b1bf7f8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> a79276d2486fc1122a8b084ea0614cdae759f88c 
> 
> Diff: https://reviews.apache.org/r/21247/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Job page with only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
> Showing config for only one config group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
> Job page with multiple config groups
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
> Viewing one of the groups from config bar
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
> Viewing multiple configs at the same time
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
> screenshot of really small group
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Review Request 21402: Add python checkstyle hooks.

[Brian Wickman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21402/
---

Review request for Aurora, Jake Farrell and Kevin Sweeney.


Bugs: AURORA-149
https://issues.apache.org/jira/browse/AURORA-149


Repository: aurora


Description
---

Consolidates python checks into build-support/python.
Adds checkstyle tool, which runs pep8, pyflakes and a number of general code 
cleanliness plugins.

Should I add the proposed style guide in this review or is a subsequent review 
fine?  It's essentially 2sp indent, 100-col lines, naming conventions + a few 
things to make life easy in a 2.x+3.x codebase.  These latter plugins can be 
omitted via the "-n" option in the checkstyle tool.

I have a separate branch that goes through and fixes all the checkstyle errors. 
 It's a ~1000 line diff but mostly minor changes.


Diffs
-

  .gitignore cd7bc6dd122fd3490568fe5b2a68d59cfff7fbff 
  build-support/hooks/pre-commit 21e2b060bc6293d259f8da1d8ac75a214b74ac1a 
  build-support/isort a5378f1e976c043b23cde62ec5c2581757e35d4f 
  build-support/isort-check 4129deb322a1cf7dba32eeec31238cf70648356a 
  build-support/isort-run c528b9a45e0b9281e0e295b55b3cd762b14c8b07 
  build-support/python/checkstyle PRE-CREATION 
  build-support/python/checkstyle-check PRE-CREATION 
  src/main/python/apache/aurora/client/cli/task.py 
1bd746da2f39f78b1497701f69777ca3ea6b70ea 

Diff: https://reviews.apache.org/r/21402/diff/


Testing
---

mba=aurora=; build-support/python/checkstyle-check src/main/python/ | grep 
"^\w" | awk '{ print $1 }' | sort | uniq -c | sort -rn
 120 E251:ERROR
  90 F401:ERROR
  86 T001:ERROR
  65 E221:ERROR
  28 T800:WARNING
  27 T301:ERROR
  22 T302:ERROR
  20 T607:ERROR
  19 E261:ERROR
  17 E303:ERROR
  14 E501:ERROR
  13 F841:ERROR
  11 E226:ERROR
   8 F821:ERROR
   6 T803:ERROR
   6 T802:WARNING
   4 T000:ERROR
   4 F403:ERROR
   4 E712:ERROR
   3 E241:ERROR
   3 E203:ERROR
   3 E202:ERROR
   2 T801:ERROR
   2 T602:ERROR
   2 T200:ERROR
   2 T100:ERROR
   2 T002:ERROR
   2 E711:ERROR
   2 E201:ERROR
   1 E231:ERROR
   1 E222:ERROR
   1 E122:ERROR


Thanks,

Brian Wickman

Review Request 21426: Added a nav bar with Aurora logo.

[Suman Karumuri]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21426/
---

Review request for Aurora, David McLaughlin and Bill Farner.


Bugs: AURORA-381
https://issues.apache.org/jira/browse/AURORA-381


Repository: aurora


Description
---

Added a nav bar with Aurora logo.


Diffs
-

  src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
dc515ac818a9af36522bb07a125cd92ff9230fa2 
  src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
817bd55a522f44831e81d0f2afe8e23bbfdbf52d 

Diff: https://reviews.apache.org/r/21426/diff/


Testing
---

Tested on local laptop.


File Attachments


Job page with nav bar
  
https://reviews.apache.org/media/uploaded/files/2014/05/14/a4338a79-72ba-4485-b0b0-86a8fdf23bca__Screen_Shot_2014-05-13_at_8.40.19_PM.png
home page with nav bar
  
https://reviews.apache.org/media/uploaded/files/2014/05/14/81c12d66-34e5-4c24-b432-85e64eb6cdc6__Screen_Shot_2014-05-13_at_8.40.24_PM.png
nav bar with a narrow page
  
https://reviews.apache.org/media/uploaded/files/2014/05/14/ef23cf5d-561a-4ae3-aad2-4321b1dd08e2__Screen_Shot_2014-05-13_at_8.40.48_PM.png


Thanks,

Suman Karumuri

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42886
---



src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java


Should have caught this before, but this should be named _interval rather 
than _rate



src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java


This begs for test coverage.  Otherwise the bug fix doesn't hold much water.


- Bill Farner


On May 12, 2014, 10:58 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 12, 2014, 10:58 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> bc900304ac6416442ae15be77fa997d8a18e7648 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 21407: Add JSHint to our build

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21407/
---

Review request for Aurora, Suman Karumuri and Bill Farner.


Bugs: AURORA-228
https://issues.apache.org/jira/browse/AURORA-228


Repository: aurora


Description
---

Add JSHint to our build.

Right now it does not break the build when JSHint fails. I'd like to file a 
separate review for this. Happy to do it as part of this if you'd prefer.


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 

Diff: https://reviews.apache.org/r/21407/diff/


Testing
---

$ ./gradlew build
:about
:bootstrapThrift UP-TO-DATE
:generateSources UP-TO-DATE
:compileGeneratedJava UP-TO-DATE
:processGeneratedResources UP-TO-DATE
:generatedClasses UP-TO-DATE
:compileJava UP-TO-DATE
:processResources
:classes
:jar
:assemble
:jsHint
Use the function form of "use strict". 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js:16:1)
> 'use strict';

Mixed double and single quotes. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js:22:35)
> $routeProvider.when("/scheduler",

Mixed double and single quotes. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js:25:41)
> $routeProvider.when("/scheduler/:role",

Mixed double and single quotes. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js:28:54)
> $routeProvider.when("/scheduler/:role/:environment",

Mixed double and single quotes. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js:31:59)
> $routeProvider.when("/scheduler/:role/:environment/:job",

Use the function form of "use strict". 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:16:1)
> 'use strict';

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:201:9)
> + stats.activeTaskCount

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:202:9)
> + stats.finishedTaskCount

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:203:9)
> + stats.failedTaskCount;

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:229:9)
> + pad(d.getUTCDate()) + ' '

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:230:9)
> + pad(d.getUTCHours()) + ':'

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:231:9)
> + pad(d.getUTCMinutes()) + ':'

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:232:9)
> + pad(d.getUTCSeconds())

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:233:9)
> + ' UTC ('

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:234:9)
> + pad(d.getMonth() + 1) + '/'

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:235:9)
> + pad(d.getDate()) + ' '

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:236:9)
> + pad(d.getHours()) + ':'

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:237:9)
> + pad(d.getMinutes()) + ':'

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:238:9)
> + pad(d.getSeconds())

Bad line breaking before '+'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:239:9)
> + ' local)';

Bad line breaking before '?'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:372:9)
> ? addColumn(2, taskColumns, taskIdColumn)

Bad line breaking before '?'. 
(/Users/dmclaughlin/t/incubator-aurora/src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js:376:9)
> ? addColumn(3, completedTaskColumns, taskIdColumn) :

Bad line breaking before '?'. 
(/U

Re: Review Request 21247: Add config grouping visualisation to job page

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/
---

(Updated May 14, 2014, 1:30 a.m.)


Review request for Aurora, Suman Karumuri and Bill Farner.


Changes
---

The config bar is now always on, Suman got a +1 on his viewpoint from a design 
resource in Seattle so I'll considered myself overruled. 

All of the interaction and event handlers for my directive have been moved 
inside the directive itself. The controller now only has code to munge the data 
into the correct format. Now any controller which wants to reuse this directive 
just needs to get the data into the same format - at which point we should move 
that logic into a service. 


Bugs: AURORA-378
https://issues.apache.org/jira/browse/AURORA-378


Repository: aurora


Description
---

Replace the button to show/hide configs with a bar when there are multiple. 
Tidied up the display of the configuration data. 


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
983101277ffbd1c4017b29f4c86e61315f1bcc78 
  src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
dc515ac818a9af36522bb07a125cd92ff9230fa2 
  src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
cd78c20bee7891a6cdfd19943a6f7e8a9dce33df 
  src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
7c80bd769b7e80bef0822846166959925b1bf7f8 
  src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
a79276d2486fc1122a8b084ea0614cdae759f88c 

Diff: https://reviews.apache.org/r/21247/diff/


Testing
---


File Attachments


Job page with only one config group
  
https://reviews.apache.org/media/uploaded/files/2014/05/08/908570f2-bef2-4370-bf08-1b5c7400c3c6__Screen_Shot_2014-05-08_at_4.26.02_PM.png
Showing config for only one config group
  
https://reviews.apache.org/media/uploaded/files/2014/05/08/1424f2c3-cf66-4ce4-a595-dc3f7647d6d2__Screen_Shot_2014-05-08_at_4.26.12_PM.png
Job page with multiple config groups
  
https://reviews.apache.org/media/uploaded/files/2014/05/08/bc9c1616-3c71-4bcc-ab40-fefc56d5e19a__Screen_Shot_2014-05-08_at_4.25.21_PM.png
Viewing one of the groups from config bar
  
https://reviews.apache.org/media/uploaded/files/2014/05/08/6b8e6077-c8c8-48c9-94e9-732e782204b6__Screen_Shot_2014-05-08_at_4.25.36_PM.png
Viewing multiple configs at the same time
  
https://reviews.apache.org/media/uploaded/files/2014/05/08/b7b2cba1-034b-43e6-9eaa-3ba1a78f87f5__Screen_Shot_2014-05-08_at_4.25.42_PM.png
screenshot of really small group
  
https://reviews.apache.org/media/uploaded/files/2014/05/12/2d7999ac-ba1f-45b0-bf9e-99f9c7a10009__Screen_Shot_2014-05-12_at_1.27.59_PM.png


Thanks,

David McLaughlin

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Maxim Khutornenko]

> On May 14, 2014, 12:42 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java, line 198
> > 
> >
> > This is problematic in two ways - it synthetically slows the build by 5 
> > seconds, and can never be sped up.  It also will be flaky depending on the 
> > behavior of the machine running tests.
> > 
> > I'm actually not sure there's a clean way to cover this through 
> > SchedulerIT.  Ideally the test coverage is narrow enough to mock the 
> > ScheduledExecutorService and test without actually waiting for the delay.  
> > I think this is doable within the sla module without too much plumbing (you 
> > just need to be able to inject the ScheduledExecutorService to SlaModule).
> 
> Maxim Khutornenko wrote:
> This is actually 5 msec not seconds. I could set it even lower, just 
> wanted to control the CPU load. Also, the awaitSlaReady() loops at 50 msec 
> interval and bails out immediately after detecting a specified sla stat.
> 
> Bill Farner wrote:
> Ah, my mistake.  The flakiness concern remains, though.  Making 
> assumptions about thread scheduling is likely to fail at some point.

Not sure I get your point. The SchedulerIT is already heavily multithreaded in 
the way it detects the running scheduler (including the use of executors). The 
awaidSlaReady() does not kick in until all other checks are done and the 
scheduler is up. By that time, the SchedulerActive event is already fired and 
the SLA thread most likely done a few iterations (given the 5 msec interval). 
I'd say this test is quite safe given it's 10 sec timeout on the callable get.  


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42920
---


On May 13, 2014, 10:09 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 13, 2014, 10:09 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> 13449ccae12bd906a4d948f4b4a83cf755eeca77 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> df5a5da120b754f7da5122feed99039c6ce7b613 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Bill Farner]

> On May 14, 2014, 12:42 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java, line 198
> > 
> >
> > This is problematic in two ways - it synthetically slows the build by 5 
> > seconds, and can never be sped up.  It also will be flaky depending on the 
> > behavior of the machine running tests.
> > 
> > I'm actually not sure there's a clean way to cover this through 
> > SchedulerIT.  Ideally the test coverage is narrow enough to mock the 
> > ScheduledExecutorService and test without actually waiting for the delay.  
> > I think this is doable within the sla module without too much plumbing (you 
> > just need to be able to inject the ScheduledExecutorService to SlaModule).
> 
> Maxim Khutornenko wrote:
> This is actually 5 msec not seconds. I could set it even lower, just 
> wanted to control the CPU load. Also, the awaitSlaReady() loops at 50 msec 
> interval and bails out immediately after detecting a specified sla stat.

Ah, my mistake.  The flakiness concern remains, though.  Making assumptions 
about thread scheduling is likely to fail at some point.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42920
---


On May 13, 2014, 10:09 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 13, 2014, 10:09 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> 13449ccae12bd906a4d948f4b4a83cf755eeca77 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> df5a5da120b754f7da5122feed99039c6ce7b613 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Maxim Khutornenko]

> On May 14, 2014, 12:42 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java, line 198
> > 
> >
> > This is problematic in two ways - it synthetically slows the build by 5 
> > seconds, and can never be sped up.  It also will be flaky depending on the 
> > behavior of the machine running tests.
> > 
> > I'm actually not sure there's a clean way to cover this through 
> > SchedulerIT.  Ideally the test coverage is narrow enough to mock the 
> > ScheduledExecutorService and test without actually waiting for the delay.  
> > I think this is doable within the sla module without too much plumbing (you 
> > just need to be able to inject the ScheduledExecutorService to SlaModule).

This is actually 5 msec not seconds. I could set it even lower, just wanted to 
control the CPU load. Also, the awaitSlaReady() loops at 50 msec interval and 
bails out immediately after detecting a specified sla stat. 


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42920
---


On May 13, 2014, 10:09 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 13, 2014, 10:09 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> 13449ccae12bd906a4d948f4b4a83cf755eeca77 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> df5a5da120b754f7da5122feed99039c6ce7b613 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42920
---



src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java


This is problematic in two ways - it synthetically slows the build by 5 
seconds, and can never be sped up.  It also will be flaky depending on the 
behavior of the machine running tests.

I'm actually not sure there's a clean way to cover this through 
SchedulerIT.  Ideally the test coverage is narrow enough to mock the 
ScheduledExecutorService and test without actually waiting for the delay.  I 
think this is doable within the sla module without too much plumbing (you just 
need to be able to inject the ScheduledExecutorService to SlaModule).


- Bill Farner


On May 13, 2014, 10:09 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 13, 2014, 10:09 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> 13449ccae12bd906a4d948f4b4a83cf755eeca77 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> df5a5da120b754f7da5122feed99039c6ce7b613 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 21170: Make the "task run" command accept an instances spec.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21170/
---

Review request for Aurora, David McLaughlin and Suman Karumuri.


Bugs: aurora-198
https://issues.apache.org/jira/browse/aurora-198


Repository: aurora


Description
---

Make the "task run" command accept an instances spec.

Also, add a subclass of DistributedCommandRunner that takes an instance list, 
and runs a command
only on the specific task instances in that list.


Diffs
-

  src/main/python/apache/aurora/client/api/command_runner.py 
1af6a782d6c0c419702ebfe0935eebbba0ea888a 
  src/main/python/apache/aurora/client/cli/options.py 
040c5c213798e644ba59c4ce3f9c4fe8868af2d5 
  src/main/python/apache/aurora/client/cli/task.py 
a162b86e465991f14160ceff465ddd7d110b57a3 
  src/test/python/apache/aurora/client/cli/test_task_run.py 
fba812efc2292be3d66cb821f0877b60afa28072 

Diff: https://reviews.apache.org/r/21170/diff/


Testing
---

[sun-wukong incubator-aurora (run_with_instances)]$ ./pants 
src/test/python/apache/aurora/client/cli:task
Build operating on targets: 
OrderedSet([PythonTests(src/test/python/apache/aurora/client/cli/BUILD:task)])
== test session starts 
===
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 3 items

src/test/python/apache/aurora/client/cli/test_task_run.py ...

 3 passed in 0.64 seconds 

src.test.python.apache.aurora.client.cli.task   
.   SUCCESS
[sun-wukong incubator-aurora (run_with_instances)]$


Thanks,

Mark Chu-Carroll

Re: Review Request 21297: Adding UpdateConfig value checks.

[Brian Wickman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42914
---

Ship it!



src/main/python/apache/aurora/client/config.py


make this a named constant somewhere, e.g. STATUS_UPDATE_DELIVERY_WINDOW

just so it's more self documenting to a casual reader.  i guess this will 
also mean you need to parameterize it in WATCH_SECS_INSUFFICIENT_ERROR


- Brian Wickman


On May 13, 2014, 10:33 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> ---
> 
> (Updated May 13, 2014, 10:33 p.m.)
> 
> 
> Review request for Aurora and Brian Wickman.
> 
> 
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 350d84c97fac9f1dd834bf509a160faaacb26386 
>   src/main/python/apache/aurora/config/__init__.py 
> 74e6b21ef905221cb3beb974a540325937480f2e 
>   src/main/python/apache/aurora/config/schema/base.py 
> 61a6680f8b1a463055ccd6d318cc540aca166684 
>   src/test/python/apache/aurora/client/cli/util.py 
> e17f256fc5d17f251e8f2cb4d94233d2d6c09897 
>   src/test/python/apache/aurora/client/commands/util.py 
> c28096fe89c9af6d5518bc0f8f871e886ca12e7f 
>   src/test/python/apache/aurora/client/test_config.py 
> 8ef08685c317c3f9dae799dfb6bdced7077a8778 
> 
> Diff: https://reviews.apache.org/r/21297/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21273: Add a "config" noun with a "list" verb to list jobs defined in a config file.

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21273/#review42836
---



src/main/python/apache/aurora/client/cli/config.py


Is it just me or a lot of these imports unused?


- David McLaughlin


On May 9, 2014, 6:05 p.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21273/
> ---
> 
> (Updated May 9, 2014, 6:05 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-403
> https://issues.apache.org/jira/browse/aurora-403
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a "config" noun with a "list" verb to list jobs defined in a config file.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/BUILD 
> 0c5a8c5cd11ad29b43027c0556710018b6ee3adc 
>   src/main/python/apache/aurora/client/cli/client.py 
> f7bafca8285ba5779ee185273d4843995a241b70 
>   src/main/python/apache/aurora/client/cli/config.py PRE-CREATION 
>   src/test/python/apache/aurora/client/cli/BUILD 
> 9766b3bdd0f5f552349453b6724573d43ddb02e5 
>   src/test/python/apache/aurora/client/cli/test_config_noun.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21273/diff/
> 
> 
> Testing
> ---
> 
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_bridge.py 
> 
> === 4 passed in 0.04 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 9 items
> 
> src/test/python/apache/aurora/client/cli/test_command_hooks.py .
> 
> === 9 passed in 0.80 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_help.py .
> 
> === 5 passed in 0.65 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 37 items
> 
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py 
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ..
> src/test/python/apache/aurora/client/cli/test_open.py .
> src/test/python/apache/aurora/client/cli/test_restart.py ...
> src/test/python/apache/aurora/client/cli/test_status.py ...
> src/test/python/apache/aurora/client/cli/test_update.py ...
> 
> == 37 passed in 2.10 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_config_noun.py ...
> 
> === 3 passed in 0.66 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 1 items
> 
> src/test/python/apache/aurora/client/cli/test_logging.py .
> 
> === 1 passed in 0.67 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_plugins.py ...
> 
> === 3 passed in 0.73 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_quota.py 
> 
> === 4 passed in 0.88 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_sla.py .
> 
> =

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Maxim Khutornenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/
---

(Updated May 13, 2014, 10:09 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-410
https://issues.apache.org/jira/browse/AURORA-410


Repository: aurora


Description
---

Starting SLA calculations on SchedulerActive event.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
13449ccae12bd906a4d948f4b4a83cf755eeca77 
  src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
8a0707ec2dfb695847d93a6e60adbbb488c22195 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
df5a5da120b754f7da5122feed99039c6ce7b613 

Diff: https://reviews.apache.org/r/21349/diff/


Testing
---

gradle build
gradle run


Thanks,

Maxim Khutornenko

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42889
---

Ship it!


Ship It!

- Bill Farner


On May 13, 2014, 9:18 p.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 9:18 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 21297: Adding UpdateConfig value checks.

[Maxim Khutornenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
---

(Updated May 13, 2014, 10:33 p.m.)


Review request for Aurora and Brian Wickman.


Changes
---

CR comments.


Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404


Repository: aurora


Description
---

Adding checks for watch_secs, initial_interval_secs and restart_threshold.


Diffs (updated)
-

  src/main/python/apache/aurora/client/config.py 
350d84c97fac9f1dd834bf509a160faaacb26386 
  src/main/python/apache/aurora/config/__init__.py 
74e6b21ef905221cb3beb974a540325937480f2e 
  src/main/python/apache/aurora/config/schema/base.py 
61a6680f8b1a463055ccd6d318cc540aca166684 
  src/test/python/apache/aurora/client/cli/util.py 
e17f256fc5d17f251e8f2cb4d94233d2d6c09897 
  src/test/python/apache/aurora/client/commands/util.py 
c28096fe89c9af6d5518bc0f8f871e886ca12e7f 
  src/test/python/apache/aurora/client/test_config.py 
8ef08685c317c3f9dae799dfb6bdced7077a8778 

Diff: https://reviews.apache.org/r/21297/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 21297: Adding UpdateConfig value checks.

[Maxim Khutornenko]

> On May 13, 2014, 9:19 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/config.py, line 130
> > 
> >
> > curious if we should be adding a delta here?  if we just do watch_secs 
> > >= initial_interval_secs, then we'll race with that first health check 
> > delivery from the executor -> scheduler -> client.
> > 
> > 5s?  not sure.  what do you think?

I was split on that too. Feels like the primary goal here should be raising 
awareness rather than giving any guarantees. That said, 5s might be a nice to 
have safety default to provide. Done.


> On May 13, 2014, 9:19 p.m., Brian Wickman wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 178
> > 
> >
> > 2 newlines between top level tests

Done.


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42887
---


On May 10, 2014, 1:15 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> ---
> 
> (Updated May 10, 2014, 1:15 a.m.)
> 
> 
> Review request for Aurora and Brian Wickman.
> 
> 
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 350d84c97fac9f1dd834bf509a160faaacb26386 
>   src/main/python/apache/aurora/config/__init__.py 
> 74e6b21ef905221cb3beb974a540325937480f2e 
>   src/main/python/apache/aurora/config/schema/base.py 
> 61a6680f8b1a463055ccd6d318cc540aca166684 
>   src/test/python/apache/aurora/client/cli/util.py 
> e17f256fc5d17f251e8f2cb4d94233d2d6c09897 
>   src/test/python/apache/aurora/client/commands/util.py 
> c28096fe89c9af6d5518bc0f8f871e886ca12e7f 
>   src/test/python/apache/aurora/client/test_config.py 
> 8ef08685c317c3f9dae799dfb6bdced7077a8778 
> 
> Diff: https://reviews.apache.org/r/21297/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21349: Starting SLA calculations on SchedulerActive event.

[Maxim Khutornenko]

> On May 13, 2014, 9:15 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java, line 60
> > 
> >
> > Should have caught this before, but this should be named _interval 
> > rather than _rate

Changed.


> On May 13, 2014, 9:15 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java, line 99
> > 
> >
> > This begs for test coverage.  Otherwise the bug fix doesn't hold much 
> > water.

Thanks for pushing back. Moved SlaModule() instantiation into SchedulerMain to 
make it testable in SchedulerIT.


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21349/#review42886
---


On May 12, 2014, 10:58 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> ---
> 
> (Updated May 12, 2014, 10:58 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
> https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> bc900304ac6416442ae15be77fa997d8a18e7648 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 
> 8a0707ec2dfb695847d93a6e60adbbb488c22195 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21273: Add a "config" noun with a "list" verb to list jobs defined in a config file.

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21273/#review42897
---

Ship it!


Ship It!

- David McLaughlin


On May 13, 2014, 6:09 p.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21273/
> ---
> 
> (Updated May 13, 2014, 6:09 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-403
> https://issues.apache.org/jira/browse/aurora-403
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a "config" noun with a "list" verb to list jobs defined in a config file.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/BUILD 
> 0c5a8c5cd11ad29b43027c0556710018b6ee3adc 
>   src/main/python/apache/aurora/client/cli/client.py 
> f7bafca8285ba5779ee185273d4843995a241b70 
>   src/main/python/apache/aurora/client/cli/config.py PRE-CREATION 
>   src/main/python/apache/aurora/client/cli/options.py 
> 040c5c213798e644ba59c4ce3f9c4fe8868af2d5 
>   src/test/python/apache/aurora/client/cli/BUILD 
> 9766b3bdd0f5f552349453b6724573d43ddb02e5 
>   src/test/python/apache/aurora/client/cli/test_config_noun.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21273/diff/
> 
> 
> Testing
> ---
> 
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_bridge.py 
> 
> === 4 passed in 0.04 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 9 items
> 
> src/test/python/apache/aurora/client/cli/test_command_hooks.py .
> 
> === 9 passed in 0.80 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_help.py .
> 
> === 5 passed in 0.65 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 37 items
> 
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py 
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ..
> src/test/python/apache/aurora/client/cli/test_open.py .
> src/test/python/apache/aurora/client/cli/test_restart.py ...
> src/test/python/apache/aurora/client/cli/test_status.py ...
> src/test/python/apache/aurora/client/cli/test_update.py ...
> 
> == 37 passed in 2.10 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_config_noun.py ...
> 
> === 3 passed in 0.66 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 1 items
> 
> src/test/python/apache/aurora/client/cli/test_logging.py .
> 
> === 1 passed in 0.67 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_plugins.py ...
> 
> === 3 passed in 0.73 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_quota.py 
> 
> === 4 passed in 0.88 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_sla.py .
> 
> === 5 passed in 0.72

Re: Review Request 21354: Fixed duration calculation in the job page.

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21354/#review42896
---

Ship it!


Ship It!

- David McLaughlin


On May 13, 2014, 12:58 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21354/
> ---
> 
> (Updated May 13, 2014, 12:58 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Mark Chu-Carroll.
> 
> 
> Bugs: AURORA-411
> https://issues.apache.org/jira/browse/AURORA-411
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixed duration calculation in the job page.
> 
> 
> Diffs
> -
> 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 768c4fe0db2813c50400370390a34cd428f88df5 
> 
> Diff: https://reviews.apache.org/r/21354/diff/
> 
> 
> Testing
> ---
> 
> Tested locally on laptop. Attached screenshot.
> 
> 
> File Attachments
> 
> 
> Now showing correct duration
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/13/f3afd556-c062-4829-ba9c-cd30d04b74fc__Screen_Shot_2014-05-12_at_5.49.51_PM.png
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Re: Review Request 21297: Adding UpdateConfig value checks.

[Brian Wickman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42887
---



src/main/python/apache/aurora/client/config.py


curious if we should be adding a delta here?  if we just do watch_secs >= 
initial_interval_secs, then we'll race with that first health check delivery 
from the executor -> scheduler -> client.

5s?  not sure.  what do you think?



src/test/python/apache/aurora/client/test_config.py


2 newlines between top level tests


- Brian Wickman


On May 10, 2014, 1:15 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> ---
> 
> (Updated May 10, 2014, 1:15 a.m.)
> 
> 
> Review request for Aurora and Brian Wickman.
> 
> 
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 350d84c97fac9f1dd834bf509a160faaacb26386 
>   src/main/python/apache/aurora/config/__init__.py 
> 74e6b21ef905221cb3beb974a540325937480f2e 
>   src/main/python/apache/aurora/config/schema/base.py 
> 61a6680f8b1a463055ccd6d318cc540aca166684 
>   src/test/python/apache/aurora/client/cli/util.py 
> e17f256fc5d17f251e8f2cb4d94233d2d6c09897 
>   src/test/python/apache/aurora/client/commands/util.py 
> c28096fe89c9af6d5518bc0f8f871e886ca12e7f 
>   src/test/python/apache/aurora/client/test_config.py 
> 8ef08685c317c3f9dae799dfb6bdced7077a8778 
> 
> Diff: https://reviews.apache.org/r/21297/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/#review42427
---



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Prefer 'static final' for constants, and SNAKE_CASE naming.  Name 
convention would be something like ROW_TO_LOCK.

Also, prefer to place constants near where they're used.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


extra newline



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


You can remove @Transactional from all of these, they should reside only in 
DbStorage.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Please either address TODO or leave a more permanent comment explaining the 
thought process for swallowing the exception.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


remove TODO for now.

However, FYI our convention is to always assign TODOs:

  TODO(davmclau): xxx.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Fits on a single line (barely)



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


Can you leave the fully-qualified class name of JdbcHelper to disambiguate? 
 Also, please explain why we need to do this.  (Reminder - it was because 
mybatis' default uses a URL that H2 can't use.)



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


You might even want to inline the magic strings here, with a good comment.  
This would keep the workaround in one place:

  // We would ideally just install the mybatis-provided helper here:
  //   install(JdbcHelper.H2_IN_MEMORY_PRIVATE);
  // Unfortunately, the H2 URL used in that constant is invalid as
  // far as H2 is concerned.  As a workaround, we clone the JdbcHelper
  // behavior here.
  
bindConstant().annotatedWith(named("JDBC.driver")).to(Driver.class.getName());
  bindConstant().annotatedWith(named("JDBC.url")).to("jdbc:h2:mem:");

If you choose to go this route, it obviates a few of the above comments.



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


A comment here would help the drive-by reader not need to drill into 
mybatis for further understanding:

// The environment ID allows SQL templates to expand differently depending 
on the
// environment used (e.g. different databases).  Since we only intend to 
use H2,
// and there is no lock-in, we use an unnamed environment.



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


// Full auto-mapping enables population of nested objects.
// Docs on settings can be found here:
// http://mybatis.github.io/mybatis-3/configuration.html#settings



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


please doc, and be sure to mention that this class is in a migratory phase, 
taking over for MemStorage.



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


When the method signature wraps, our convention is to leave an empty line 
to pad the signature:

  public T longMethodSignature(Args args)
  throws ExceptionType {

// first body line
  }



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


@Transactional



src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java


please doc.  ditto for methods



src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java


doc interface + methods



src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java


More details would be nice (if you copy, please wrap to 100 cols):

  Temporary module to wire the two

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/
---

(Updated May 13, 2014, 2:18 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

reverted change to rbt search path.


Repository: aurora


Description
---

Add support for custom project to list-missing-shipits.
Search for rbt on PATH.


Diffs (updated)
-

  build-support/tools/list-missing-shipits 
33a4c412092a5a9a21ed8e5aaee69989535a12e7 

Diff: https://reviews.apache.org/r/21386/diff/


Testing
---

Ran script against project Mesos.


Thanks,

Dominic Hamon

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Kevin Sweeney]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/#review42858
---

Ship it!


Ship It!

- Kevin Sweeney


On May 13, 2014, 11:13 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21352/
> ---
> 
> (Updated May 13, 2014, 11:13 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-416
> https://issues.apache.org/jira/browse/AURORA-416
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Since this would have been caught by findbugs when introduced, i decided to 
> configure findbugs on our build as added prevention going forward.
> 
> I also addressed a few findbugs warnings which took fewer characters to 
> exclude than to fix.
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   config/findbugs/excludeFilter.xml PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
> ada5eafdf484f733c07277754833313d5e7bdc4b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
>   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
> e23ab5cd49a52f3004baa4e30462c6b028931371 
>   src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
> a9b85d0983dcfee89101a5e774ba86ee11708c68 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
> de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  47d2fd6d8a34cb14d68894e14c147709373e3572 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> e212174ed089fdcf28fa679318fe216917a40b99 
> 
> Diff: https://reviews.apache.org/r/21352/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Jake Farrell]

> On May 13, 2014, 6:07 p.m., Bill Farner wrote:
> > build-support/tools/list-missing-shipits, line 94
> > 
> >
> > At a quick glance, it's not obvious how this relates to plumbing the 
> > 'project' argument.  Was there another issue you bumped into?  Is this so 
> > you can use the script outside the repo?
> 
> Dominic Hamon wrote:
> yes - other projects don't have rbt in the specific path the script was 
> pointing at.
> 
> I considered keeping this as the default if 'rbt' isn't found on the path 
> but thought that might be flaky.
> 
> Kevin Sweeney wrote:
> FWIW I'd highly recommend the aurora approach (self-bootstrapping, 
> isolated rbt) over requiring a global installation of it with pip, since you 
> get to explicitly specify the target version and it's one less barrier to 
> entry for new contributors. To get this in another repo you'd want to copy 
> ./rbt, ./build-support/virtualenv, and 
> ./build-support/tools/list-missing-shipits.
> 
> Dominic Hamon wrote:
> that sounds like a great plan, especially with all the recent churn on 
> our own post-review script. Perhaps we should compare notes on each approach.

Agree with Kevin on this, we should keep the set path for our version of rbt 
which runs within venv and bootstraps itself (this helps avoid issues like 
Mesos ran into with the latest version of rbt). It might be worth looking into 
as a sub command within rbt to avoid having to call externally


- Jake


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42853
---


On May 13, 2014, 6:07 p.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 6:07 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/
---

(Updated May 13, 2014, 6:03 p.m.)


Review request for Aurora.


Changes
---

s/mesos/Aurora/ for review group.


Repository: aurora


Description
---

Add support for custom project to list-missing-shipits.
Search for rbt on PATH.


Diffs
-

  build-support/tools/list-missing-shipits 
33a4c412092a5a9a21ed8e5aaee69989535a12e7 

Diff: https://reviews.apache.org/r/21386/diff/


Testing
---

Ran script against project Mesos.


Thanks,

Dominic Hamon

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/
---

(Updated May 13, 2014, 11 a.m.)


Review request for Aurora.


Changes
---

changed groups.


Repository: aurora


Description
---

Add support for custom project to list-missing-shipits.
Search for rbt on PATH.


Diffs
-

  build-support/tools/list-missing-shipits 
33a4c412092a5a9a21ed8e5aaee69989535a12e7 

Diff: https://reviews.apache.org/r/21386/diff/


Testing
---

Ran script against project Mesos.


Thanks,

Dominic Hamon

Re: Review Request 21402: Add python checkstyle hooks.

[Brian Wickman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21402/
---

(Updated May 13, 2014, 8:04 p.m.)


Review request for Aurora, Jake Farrell and Kevin Sweeney.


Changes
---

Add "$@" to checkstyle-check so that default (--diff=master) can be overridden 
with source paths.


Bugs: AURORA-149
https://issues.apache.org/jira/browse/AURORA-149


Repository: aurora


Description
---

Consolidates python checks into build-support/python.
Adds checkstyle tool, which runs pep8, pyflakes and a number of general code 
cleanliness plugins.

Should I add the proposed style guide in this review or is a subsequent review 
fine?  It's essentially 2sp indent, 100-col lines, naming conventions + a few 
things to make life easy in a 2.x+3.x codebase.  These latter plugins can be 
omitted via the "-n" option in the checkstyle tool.

I have a separate branch that goes through and fixes all the checkstyle errors. 
 It's a ~1000 line diff but mostly minor changes.


Diffs (updated)
-

  .gitignore cd7bc6dd122fd3490568fe5b2a68d59cfff7fbff 
  build-support/hooks/pre-commit 21e2b060bc6293d259f8da1d8ac75a214b74ac1a 
  build-support/isort a5378f1e976c043b23cde62ec5c2581757e35d4f 
  build-support/isort-check 4129deb322a1cf7dba32eeec31238cf70648356a 
  build-support/isort-run c528b9a45e0b9281e0e295b55b3cd762b14c8b07 
  build-support/python/checkstyle PRE-CREATION 
  build-support/python/checkstyle-check PRE-CREATION 
  src/main/python/apache/aurora/client/cli/task.py 
1bd746da2f39f78b1497701f69777ca3ea6b70ea 

Diff: https://reviews.apache.org/r/21402/diff/


Testing
---

mba=aurora=; build-support/python/checkstyle-check src/main/python/ | grep 
"^\w" | awk '{ print $1 }' | sort | uniq -c | sort -rn
 120 E251:ERROR
  90 F401:ERROR
  86 T001:ERROR
  65 E221:ERROR
  28 T800:WARNING
  27 T301:ERROR
  22 T302:ERROR
  20 T607:ERROR
  19 E261:ERROR
  17 E303:ERROR
  14 E501:ERROR
  13 F841:ERROR
  11 E226:ERROR
   8 F821:ERROR
   6 T803:ERROR
   6 T802:WARNING
   4 T000:ERROR
   4 F403:ERROR
   4 E712:ERROR
   3 E241:ERROR
   3 E203:ERROR
   3 E202:ERROR
   2 T801:ERROR
   2 T602:ERROR
   2 T200:ERROR
   2 T100:ERROR
   2 T002:ERROR
   2 E711:ERROR
   2 E201:ERROR
   1 E231:ERROR
   1 E222:ERROR
   1 E122:ERROR


Thanks,

Brian Wickman

Re: Review Request 21354: Fixed duration calculation in the job page.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21354/#review42872
---

Ship it!


Ship It!

- Mark Chu-Carroll


On May 12, 2014, 8:58 p.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21354/
> ---
> 
> (Updated May 12, 2014, 8:58 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Mark Chu-Carroll.
> 
> 
> Bugs: AURORA-411
> https://issues.apache.org/jira/browse/AURORA-411
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixed duration calculation in the job page.
> 
> 
> Diffs
> -
> 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 768c4fe0db2813c50400370390a34cd428f88df5 
> 
> Diff: https://reviews.apache.org/r/21354/diff/
> 
> 
> Testing
> ---
> 
> Tested locally on laptop. Attached screenshot.
> 
> 
> File Attachments
> 
> 
> Now showing correct duration
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/13/f3afd556-c062-4829-ba9c-cd30d04b74fc__Screen_Shot_2014-05-12_at_5.49.51_PM.png
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Kevin Sweeney]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/#review42852
---



config/findbugs/excludeFilter.xml


You should be able to add a java namespace to the .thrift file here (one 
already exists for python)


- Kevin Sweeney


On May 12, 2014, 5:06 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21352/
> ---
> 
> (Updated May 12, 2014, 5:06 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-416
> https://issues.apache.org/jira/browse/AURORA-416
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Since this would have been caught by findbugs when introduced, i decided to 
> configure findbugs on our build as added prevention going forward.
> 
> I also addressed a few findbugs warnings which took fewer characters to 
> exclude than to fix.
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   config/findbugs/excludeFilter.xml PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
> ada5eafdf484f733c07277754833313d5e7bdc4b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
>   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
> e23ab5cd49a52f3004baa4e30462c6b028931371 
>   src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
> a9b85d0983dcfee89101a5e774ba86ee11708c68 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
> de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  47d2fd6d8a34cb14d68894e14c147709373e3572 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> e212174ed089fdcf28fa679318fe216917a40b99 
> 
> Diff: https://reviews.apache.org/r/21352/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/
---

(Updated May 13, 2014, 6:27 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Removing myself as a reviewer since i paired on this code, and to prevent lazy 
reviewer consensus on my ship it.


Bugs: AURORA-335
https://issues.apache.org/jira/browse/AURORA-335


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

Diff: https://reviews.apache.org/r/21132/diff/


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21170: Make the "task run" command accept an instances spec.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21170/
---

(Updated May 13, 2014, 3:33 p.m.)


Review request for Aurora, David McLaughlin and Suman Karumuri.


Changes
---

Rebase to master.


Bugs: aurora-198
https://issues.apache.org/jira/browse/aurora-198


Repository: aurora


Description
---

Make the "task run" command accept an instances spec.

Also, add a subclass of DistributedCommandRunner that takes an instance list, 
and runs a command
only on the specific task instances in that list.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/command_runner.py 
14f8e454ac6b8fe2c2925f834d0761c053eea337 
  src/main/python/apache/aurora/client/cli/options.py 
bf86c3abe218d28b13a6d1bc25abe4f48e999b92 
  src/main/python/apache/aurora/client/cli/task.py 
1253ece196413075c86840b633c61ebd2188a2fc 
  src/test/python/apache/aurora/client/cli/test_task_run.py 
c60189ca74bcd7152209430537ea2790e6db8457 

Diff: https://reviews.apache.org/r/21170/diff/


Testing
---

[sun-wukong incubator-aurora (run_with_instances)]$ ./pants 
src/test/python/apache/aurora/client/cli:task
Build operating on targets: 
OrderedSet([PythonTests(src/test/python/apache/aurora/client/cli/BUILD:task)])
== test session starts 
===
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 3 items

src/test/python/apache/aurora/client/cli/test_task_run.py ...

 3 passed in 0.64 seconds 

src.test.python.apache.aurora.client.cli.task   
.   SUCCESS
[sun-wukong incubator-aurora (run_with_instances)]$


Thanks,

Mark Chu-Carroll

Re: Review Request 21205: Bugfix: restart doesn't notify user about invalid max_total_failures option.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21205/
---

(Updated May 13, 2014, 3:18 p.m.)


Review request for Aurora, David McLaughlin and Suman Karumuri.


Changes
---

Rebase to master.


Bugs: aurora-399
https://issues.apache.org/jira/browse/aurora-399


Repository: aurora


Description
---

Bugfix: restart doesn't notify user about invalid max_total_failures option.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/jobs.py 
6fc08dacf1ec1242bc30512fe566a541140feef5 
  src/main/python/apache/aurora/client/commands/core.py 
4ce60351f8578b46e9f5ee1b0c04e89f8007d595 
  src/test/python/apache/aurora/client/cli/test_restart.py 
d365ef438b35cb0e87e33b6f02301236dc01fd2a 
  src/test/python/apache/aurora/client/commands/test_restart.py 
fba529f187be8ad82c187296b74d135f63164348 

Diff: https://reviews.apache.org/r/21205/diff/


Testing
---

[sun-wukong incubator-aurora (restart_max)]$ ./pants 
src/test/python/apache/aurora/client:all
Build operating on targets: 
OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/BUILD:all)])
== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 2 items

src/test/python/apache/aurora/client/test_binding_helper.py ..

=== 2 passed in 0.36 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 6 items

src/test/python/apache/aurora/client/test_config.py ..

=== 6 passed in 0.45 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 6 items

src/test/python/apache/aurora/client/api/test_disambiguator.py ..

=== 6 passed in 0.41 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 4 items

src/test/python/apache/aurora/client/api/test_job_monitor.py 

=== 4 passed in 0.39 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 6 items

src/test/python/apache/aurora/client/api/test_restarter.py ..

=== 6 passed in 0.34 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 47 items / 1 skipped

src/test/python/apache/aurora/client/api/test_scheduler_client.py 
...

= 47 passed, 1 skipped in 4.05 seconds 
==
== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 12 items

src/test/python/apache/aurora/client/api/test_instance_watcher.py 
src/test/python/apache/aurora/client/api/test_health_check.py 

=== 12 passed in 0.22 seconds 
===
== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 28 items

src/test/python/apache/aurora/client/api/test_updater.py 


=== 28 passed in 0.67 seconds 
===
== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 6 items

src/test/python/apache/aurora/client/api/test_quota_check.py ..

=== 6 passed in 0.14 seconds 

== test session starts 
==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 33 items

src/test/python/apache/aurora/client/api/test_sla.py 
.

=== 33 passed in 0.29 seconds 
===

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

> On May 13, 2014, 11:07 a.m., Bill Farner wrote:
> > build-support/tools/list-missing-shipits, line 94
> > 
> >
> > At a quick glance, it's not obvious how this relates to plumbing the 
> > 'project' argument.  Was there another issue you bumped into?  Is this so 
> > you can use the script outside the repo?

yes - other projects don't have rbt in the specific path the script was 
pointing at.

I considered keeping this as the default if 'rbt' isn't found on the path but 
thought that might be flaky.


- Dominic


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42853
---


On May 13, 2014, 11:07 a.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 11:07 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

> On May 13, 2014, 11:07 a.m., Bill Farner wrote:
> > build-support/tools/list-missing-shipits, line 94
> > 
> >
> > At a quick glance, it's not obvious how this relates to plumbing the 
> > 'project' argument.  Was there another issue you bumped into?  Is this so 
> > you can use the script outside the repo?
> 
> Dominic Hamon wrote:
> yes - other projects don't have rbt in the specific path the script was 
> pointing at.
> 
> I considered keeping this as the default if 'rbt' isn't found on the path 
> but thought that might be flaky.
> 
> Kevin Sweeney wrote:
> FWIW I'd highly recommend the aurora approach (self-bootstrapping, 
> isolated rbt) over requiring a global installation of it with pip, since you 
> get to explicitly specify the target version and it's one less barrier to 
> entry for new contributors. To get this in another repo you'd want to copy 
> ./rbt, ./build-support/virtualenv, and 
> ./build-support/tools/list-missing-shipits.

that sounds like a great plan, especially with all the recent churn on our own 
post-review script. Perhaps we should compare notes on each approach.


- Dominic


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42853
---


On May 13, 2014, 11:07 a.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 11:07 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[David McLaughlin]

> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 54
> > 
> >
> > Formatting nit:
> > 
> >   TODO(davmclau)
> > 
> > as opposed to:
> > 
> >   TODO (davmclau)
> > 
> > Not a big deal, but makes grepping easier

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 59
> > 
> >
> > period at the end to make it clear that your thought wasn't cut off.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 77
> > 
> >
> > This is just as readable if named TO_ROW, and as a bonus the signature 
> > fits on one line.

done.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 96
> > 
> >
> > s/TODO /TODO/

ack. 


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java, 
> > line 29
> > 
> >
> > Please punctuate sentences to make it clear words weren't accidentally 
> > lopped off.
> > 
> > Here and elsewhere in this diff.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java, 
> > line 34
> > 
> >
> > s/  / /

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java, 
> > line 23
> > 
> >
> > s/This class represents the/The/

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java, 
> > line 27
> > 
> >
> > + Note that this must be reworked if other fields are introduced into 
> > LockKey.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml, 
> > line 19
> > 
> >
> > Consider extracting a fragment for the WHERE clause (used 2x).

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, 
> > line 8
> > 
> >
> > +2 indent

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, 
> > line 21
> > 
> >
> > +2 indent

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 63
> > 
> >
> > s/throws RuntimeException //

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 70
> > 
> >
> > MutateWork.Quiet allows you to remove the Exception from your 
> > signature.  Here and below.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 73
> > 
> >
> > colon should be space-padded:
> > 
> >   for (ILock lock : locks) {
> > 
> > here and below

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 104
> > 
> >
> > s/final //, applies to ~all test cases

So the rule here is you don't bother applying final to variables that are only 
visible inside the method?


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 108
> > 
> >
> > this function would help cut down a bunch of redundancy in this class:
> > 
> >   private static ILock makeLock(JobKey job) {
> >

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42853
---



build-support/tools/list-missing-shipits


At a quick glance, it's not obvious how this relates to plumbing the 
'project' argument.  Was there another issue you bumped into?  Is this so you 
can use the script outside the repo?


- Bill Farner


On May 13, 2014, 6:07 p.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 6:07 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Kevin Sweeney]

> On May 13, 2014, 11:07 a.m., Bill Farner wrote:
> > build-support/tools/list-missing-shipits, line 94
> > 
> >
> > At a quick glance, it's not obvious how this relates to plumbing the 
> > 'project' argument.  Was there another issue you bumped into?  Is this so 
> > you can use the script outside the repo?
> 
> Dominic Hamon wrote:
> yes - other projects don't have rbt in the specific path the script was 
> pointing at.
> 
> I considered keeping this as the default if 'rbt' isn't found on the path 
> but thought that might be flaky.

FWIW I'd highly recommend the aurora approach (self-bootstrapping, isolated 
rbt) over requiring a global installation of it with pip, since you get to 
explicitly specify the target version and it's one less barrier to entry for 
new contributors. To get this in another repo you'd want to copy ./rbt, 
./build-support/virtualenv, and ./build-support/tools/list-missing-shipits.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/#review42853
---


On May 13, 2014, 11:07 a.m., Dominic Hamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21386/
> ---
> 
> (Updated May 13, 2014, 11:07 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for custom project to list-missing-shipits.
> Search for rbt on PATH.
> 
> 
> Diffs
> -
> 
>   build-support/tools/list-missing-shipits 
> 33a4c412092a5a9a21ed8e5aaee69989535a12e7 
> 
> Diff: https://reviews.apache.org/r/21386/diff/
> 
> 
> Testing
> ---
> 
> Ran script against project Mesos.
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>

Re: Review Request 19627: Updates documentation files for markdown consistency

[Bill Farner]

> On April 24, 2014, 6:19 p.m., Bill Farner wrote:
> > Dave - can you push this on a branch to github so we can see it rendered 
> > there?

Ping?


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19627/#review41353
---


On April 24, 2014, 12:43 a.m., Dave Lester wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19627/
> ---
> 
> (Updated April 24, 2014, 12:43 a.m.)
> 
> 
> Review request for Aurora and Mark Chu-Carroll.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Updates documentation to add headers to pages that needed them, cleans up 
> whitespace, and makes markdown for headers consistent.
> 
> 
> Diffs
> -
> 
>   docs/README.md b670c04a8cf398e14e7fad7d00480a973d37968b 
>   docs/clientcommands.md adf378c49d4e6a6fbc399976c034a7af390437ec 
>   docs/configurationreference.md 9d5c340025d2d14be37ef7d1a67f186c8c1792db 
>   docs/configurationtutorial.md 9b27fb62ef8f92c241d59d6ae55c27ae7ca8d5a9 
>   docs/contributing.md 434116378711f40877f6f7af15af8545b5335ec0 
>   docs/deploying-aurora-scheduler.md 887bf1691068037be5f9adf4ddb9e999ec1a899b 
>   docs/developing-aurora-scheduler.md 
> 95bf47278b4cf040ed902e43a151fe9fae19bfc5 
>   docs/hooks.md 77fb95598b9f86c36979089cd4aa798044329367 
>   docs/resourceisolation.md 7e8d88d09093d85c07c84bd3d6476fc89ff21c3b 
>   docs/tutorial.md 3ccc862e7819af89d414d10325c90e10a6e49887 
>   docs/userguide.md 2fbafb5d6b340eaf37fa9dd3250b355518538f8e 
>   docs/vagrant.md 15d72bc57e2d26222ce7b664ece887c06b2c0e53 
> 
> Diff: https://reviews.apache.org/r/19627/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dave Lester
> 
>

Re: Review Request 21247: Add config grouping visualisation to job page

[Bill Farner]

> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > I think we should show the config bar even when there is one config. It 
> > will also act a visual indication to the user that all his tasks are in a 
> > consistent config.
> 
> David McLaughlin wrote:
> This is a fair suggestion, and I tried this initially. Having a 100% 
> color bar across the header of the page dominates the user's attention when 
> there is nothing for them to see.
> 
> Suman Karumuri wrote:
> I think the bar grabbing the user's attention when he goes to a page is 
> ok in this case because we are telling him that all the jobs are having the 
> same config. Once we add something else like a job status bar that needs 
> user's attention, we can consider hiding this automatically. Further, when we 
> add auto-refresh to the page, the bar would keep appearing and disappearing 
> during an update which can be jarring. So, I think we should show the bar 
> always.
> 
> David McLaughlin wrote:
> Well, there is no need for a visualisation when everything is homogenous. 
> You just need one link to show or hide the config, similar to what we have 
> right now. The purpose of the visualisation is to grab the user's attention 
> and let them know - hey this job has special instances that you may not know 
> about (failed updates, canaries, etc.). Always showing the bar is sort of 
> like an 'everything is ok alarm'.
> 
> The auto-refresh use case - in practice it would only appear when the 
> update is in progress and disappear when it is complete. If it is jarring 
> then it is so by design ("hey! look at this! something interesting is 
> happening!"). It will not flicker during the update itself. 
> 
> I'll let someone else tie-break this though. Remember that this is just 
> an initial design, when we add more features to this page and receive user 
> feedback, we will adapt to that.

I'm in favor of giving the person behind the keyboard the majority vote here.  
Sounds like David has played with both approaches and found the current diff 
most user-friendly.  Let's re-assess if we find otherwise.


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 385
> > 
> >
> > please only pass the required fields into the fields into the function. 
> > By passing in $scope, it is very unclear what inputs buildGroupSummary 
> > depends on.
> 
> David McLaughlin wrote:
> The function signature is quite clear that buildGroupSummary is a 
> function which depends on the global $scope (it both reads and writes to it - 
> this is how Angular passes around such objects to its controllers which do 
> the same thing).
> 
> David McLaughlin wrote:
> Just to be clear on this point. Take a look at getTasksForJob:
> 
> function getTasksForJob(role, environment, job) {
>...
> }
> 
> You might think this gives some form of clarity, but it does not. It's 
> confusing, because this function goes on to use the following variables via a 
> closure:
> 
>  auroraClient, $scope, taskUtil, getJobDashboardUrl, summarizeTask
> 
> 
> On the other hand, everything that buildGroupSummary needs is passed into 
> the function or created inside it. You could copy and paste this function 
> elsewhere in the code base - move it completely outside of the controller or 
> even pass it into the controller via DI and the call to this function _from 
> the controller_ would not change. I think ultimately all these functions used 
> to group code together need to be moved and this controller simplified, but I 
> want to keep this review small and do the larger cleaning up in another 
> ticket.
> 
> Suman Karumuri wrote:
> Ok. we can discuss the cleanup and code style else where.

Mind having this discussion in the abstract on the dev mailing list?  As a 
javascript noob, I find it interesting, and wouldn't mind following along.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42626
---


On May 13, 2014, 12:02 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> ---
> 
> (Updated May 13, 2014, 12:02 a.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Bugs: AURORA-378
> https://issues.apache.org/jira/browse/AURORA-378
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Replace the button to show/hide configs with a bar when there are multiple. 
> Tidied up the display of the configuration data. 
> 
> 
> Dif

Review Request 21383: Add cron schedule and deschedule calls to the scheduler API.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21383/
---

Review request for Aurora, David McLaughlin and Bill Farner.


Bugs: aurora-417
https://issues.apache.org/jira/browse/aurora-417


Repository: aurora


Description
---

Add cron schedule and deschedule calls to the scheduler API.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
226b71ce7749492abd3e1d673382668c9011a1c8 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
f33059904fb5f6fc18a3a66dfe63c059008affe3 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
9bb5c255e1118038b953fc72f0a074d461ba1fb5 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
66292dc0369941fc62719d49209edc07adc81a53 
  
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 
da6c0ff1bdfaefef2f72ad2559f4059cd44612b6 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
933a56bf3f7165fa84aedcc8d1392e32824fd487 

Diff: https://reviews.apache.org/r/21383/diff/


Testing
---

Ran all tests with gradlew; no errors. (Includes two new tests, that ensure 
that calling the new APIs causes the right internal cron methods to be called.)


Thanks,

Mark Chu-Carroll

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[Bill Farner]

> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 104
> > 
> >
> > s/final //, applies to ~all test cases
> 
> David McLaughlin wrote:
> So the rule here is you don't bother applying final to variables that are 
> only visible inside the method?

That's right.  I agree that it's somewhat inconsistent, but useful for brevity.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/#review42729
---


On May 13, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[David McLaughlin]

> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 108
> > 
> >
> > this function would help cut down a bunch of redundancy in this class:
> > 
> >   private static ILock makeLock(JobKey job) {
> > ...
> >   }
> 
> David McLaughlin wrote:
> ack. 
> 
> Since it is useful to be able to differentiate between two locks (and we 
> do this in at least one test), I'm going to add two methods. One which 
> returns a Lock and one which returns an ILock. 
> 
>

Err, scratch that - realised there was no need for the two different tokens on 
those locks. So now just makeLock. 


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/#review42729
---


On May 13, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Kevin Sweeney]

> On May 13, 2014, 11:05 a.m., Kevin Sweeney wrote:
> > config/findbugs/excludeFilter.xml, line 9
> > 
> >
> > You should be able to add a java namespace to the .thrift file here 
> > (one already exists for python)
> 
> Bill Farner wrote:
> Punting on this.  I'd rather they just weren't generated for java, tbh.

Mind dropping a TODO to exclude them in build.gradle?


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/#review42852
---


On May 13, 2014, 11:13 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21352/
> ---
> 
> (Updated May 13, 2014, 11:13 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-416
> https://issues.apache.org/jira/browse/AURORA-416
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Since this would have been caught by findbugs when introduced, i decided to 
> configure findbugs on our build as added prevention going forward.
> 
> I also addressed a few findbugs warnings which took fewer characters to 
> exclude than to fix.
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   config/findbugs/excludeFilter.xml PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
> ada5eafdf484f733c07277754833313d5e7bdc4b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
>   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
> e23ab5cd49a52f3004baa4e30462c6b028931371 
>   src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
> a9b85d0983dcfee89101a5e774ba86ee11708c68 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
> de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  47d2fd6d8a34cb14d68894e14c147709373e3572 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> e212174ed089fdcf28fa679318fe216917a40b99 
> 
> Diff: https://reviews.apache.org/r/21352/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/
---

(Updated May 13, 2014, 5:49 p.m.)


Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.


Changes
---

Review feedback. 


Bugs: AURORA-335
https://issues.apache.org/jira/browse/AURORA-335


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

Diff: https://reviews.apache.org/r/21132/diff/


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21273: Add a "config" noun with a "list" verb to list jobs defined in a config file.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21273/
---

(Updated May 13, 2014, 2:09 p.m.)


Review request for Aurora, David McLaughlin and Suman Karumuri.


Changes
---

Ran a python checkstyle tool, and removed all of the unnecessary imports it 
detected.


Bugs: aurora-403
https://issues.apache.org/jira/browse/aurora-403


Repository: aurora


Description
---

Add a "config" noun with a "list" verb to list jobs defined in a config file.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/BUILD 
0c5a8c5cd11ad29b43027c0556710018b6ee3adc 
  src/main/python/apache/aurora/client/cli/client.py 
f7bafca8285ba5779ee185273d4843995a241b70 
  src/main/python/apache/aurora/client/cli/config.py PRE-CREATION 
  src/main/python/apache/aurora/client/cli/options.py 
040c5c213798e644ba59c4ce3f9c4fe8868af2d5 
  src/test/python/apache/aurora/client/cli/BUILD 
9766b3bdd0f5f552349453b6724573d43ddb02e5 
  src/test/python/apache/aurora/client/cli/test_config_noun.py PRE-CREATION 

Diff: https://reviews.apache.org/r/21273/diff/


Testing
---

= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 4 items

src/test/python/apache/aurora/client/cli/test_bridge.py 

=== 4 passed in 0.04 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 9 items

src/test/python/apache/aurora/client/cli/test_command_hooks.py .

=== 9 passed in 0.80 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 5 items

src/test/python/apache/aurora/client/cli/test_help.py .

=== 5 passed in 0.65 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 37 items

src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
src/test/python/apache/aurora/client/cli/test_create.py 
src/test/python/apache/aurora/client/cli/test_diff.py ...
src/test/python/apache/aurora/client/cli/test_kill.py ..
src/test/python/apache/aurora/client/cli/test_open.py .
src/test/python/apache/aurora/client/cli/test_restart.py ...
src/test/python/apache/aurora/client/cli/test_status.py ...
src/test/python/apache/aurora/client/cli/test_update.py ...

== 37 passed in 2.10 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 3 items

src/test/python/apache/aurora/client/cli/test_config_noun.py ...

=== 3 passed in 0.66 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 1 items

src/test/python/apache/aurora/client/cli/test_logging.py .

=== 1 passed in 0.67 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 3 items

src/test/python/apache/aurora/client/cli/test_plugins.py ...

=== 3 passed in 0.73 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 4 items

src/test/python/apache/aurora/client/cli/test_quota.py 

=== 4 passed in 0.88 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 5 items

src/test/python/apache/aurora/client/cli/test_sla.py .

=== 5 passed in 0.72 seconds ===
= test session starts ==
platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
plugins: cov
collected 2 items

src/test/python/apache/aurora/client/cli/test_task_run.py ..

=== 2 passed in 0.71 seconds ===
Build operating on targets: 
OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/cli/BUILD:all)])
src.test.python.apache.aurora.client.cli.b

Re: Review Request 21386: Add support for custom project to list-missing-shipits

[Dominic Hamon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21386/
---

(Updated May 13, 2014, 6:07 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Repository: aurora


Description
---

Add support for custom project to list-missing-shipits.
Search for rbt on PATH.


Diffs
-

  build-support/tools/list-missing-shipits 
33a4c412092a5a9a21ed8e5aaee69989535a12e7 

Diff: https://reviews.apache.org/r/21386/diff/


Testing
---

Ran script against project Mesos.


Thanks,

Dominic Hamon

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/
---

(Updated May 13, 2014, 6:13 p.m.)


Review request for Aurora, David McLaughlin and Maxim Khutornenko.


Bugs: AURORA-416
https://issues.apache.org/jira/browse/AURORA-416


Repository: aurora


Description
---

Since this would have been caught by findbugs when introduced, i decided to 
configure findbugs on our build as added prevention going forward.

I also addressed a few findbugs warnings which took fewer characters to exclude 
than to fix.


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  config/findbugs/excludeFilter.xml PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
ada5eafdf484f733c07277754833313d5e7bdc4b 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
  src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
e23ab5cd49a52f3004baa4e30462c6b028931371 
  src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
a9b85d0983dcfee89101a5e774ba86ee11708c68 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 47d2fd6d8a34cb14d68894e14c147709373e3572 
  src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
e212174ed089fdcf28fa679318fe216917a40b99 

Diff: https://reviews.apache.org/r/21352/diff/


Testing
---

./gradlew build


Thanks,

Bill Farner

Re: Review Request 21170: Make the "task run" command accept an instances spec.

[Suman Karumuri]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21170/#review42840
---

Ship it!


Ship It!

- Suman Karumuri


On May 7, 2014, 6:48 p.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21170/
> ---
> 
> (Updated May 7, 2014, 6:48 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-198
> https://issues.apache.org/jira/browse/aurora-198
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Make the "task run" command accept an instances spec.
> 
> Also, add a subclass of DistributedCommandRunner that takes an instance list, 
> and runs a command
> only on the specific task instances in that list.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/command_runner.py 
> 1af6a782d6c0c419702ebfe0935eebbba0ea888a 
>   src/main/python/apache/aurora/client/cli/options.py 
> 040c5c213798e644ba59c4ce3f9c4fe8868af2d5 
>   src/main/python/apache/aurora/client/cli/task.py 
> a162b86e465991f14160ceff465ddd7d110b57a3 
>   src/test/python/apache/aurora/client/cli/test_task_run.py 
> fba812efc2292be3d66cb821f0877b60afa28072 
> 
> Diff: https://reviews.apache.org/r/21170/diff/
> 
> 
> Testing
> ---
> 
> [sun-wukong incubator-aurora (run_with_instances)]$ ./pants 
> src/test/python/apache/aurora/client/cli:task
> Build operating on targets: 
> OrderedSet([PythonTests(src/test/python/apache/aurora/client/cli/BUILD:task)])
> == test session starts 
> ===
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_task_run.py ...
> 
>  3 passed in 0.64 seconds 
> 
> src.test.python.apache.aurora.client.cli.task 
>   .   SUCCESS
> [sun-wukong incubator-aurora (run_with_instances)]$
> 
> 
> Thanks,
> 
> Mark Chu-Carroll
> 
>

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Bill Farner]

> On May 13, 2014, 6:05 p.m., Kevin Sweeney wrote:
> > config/findbugs/excludeFilter.xml, line 9
> > 
> >
> > You should be able to add a java namespace to the .thrift file here 
> > (one already exists for python)

Punting on this.  I'd rather they just weren't generated for java, tbh.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/#review42852
---


On May 13, 2014, 12:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21352/
> ---
> 
> (Updated May 13, 2014, 12:06 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-416
> https://issues.apache.org/jira/browse/AURORA-416
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Since this would have been caught by findbugs when introduced, i decided to 
> configure findbugs on our build as added prevention going forward.
> 
> I also addressed a few findbugs warnings which took fewer characters to 
> exclude than to fix.
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   config/findbugs/excludeFilter.xml PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
> ada5eafdf484f733c07277754833313d5e7bdc4b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
>   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
> e23ab5cd49a52f3004baa4e30462c6b028931371 
>   src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
> a9b85d0983dcfee89101a5e774ba86ee11708c68 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
> de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  47d2fd6d8a34cb14d68894e14c147709373e3572 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> e212174ed089fdcf28fa679318fe216917a40b99 
> 
> Diff: https://reviews.apache.org/r/21352/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

[David McLaughlin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21132/
---

(Updated May 13, 2014, 5:49 p.m.)


Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.


Bugs: AURORA-335
https://issues.apache.org/jira/browse/AURORA-335


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

Diff: https://reviews.apache.org/r/21132/diff/


Testing
---


Thanks,

David McLaughlin

Re: Review Request 20285: Improve documentation and testing for host maintenance API

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20285/#review42851
---


Update or discard?

- Bill Farner


On April 12, 2014, 11:45 p.m., Joe Smith wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20285/
> ---
> 
> (Updated April 12, 2014, 11:45 p.m.)
> 
> 
> Review request for Aurora, David Robinson, Kevin Sweeney, Tobias Weingartner, 
> and Bill Farner.
> 
> 
> Bugs: AURORA-318
> https://issues.apache.org/jira/browse/AURORA-318
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add way more docstrings as  well as unit tests for the HostMaintenance api.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/host_maintenance.py 
> 6a21ddeb4203a07a04360f68c4a338fe3da52c70 
>   src/test/python/apache/aurora/admin/BUILD 
> 17aac54d636024a8e131eb2e4d996c2e15401de1 
>   src/test/python/apache/aurora/admin/test_host_maintenance.py 
> 95497aea3d3f769b2460adbfe7a7b72d775ced6f 
> 
> Diff: https://reviews.apache.org/r/20285/diff/
> 
> 
> Testing
> ---
> 
> 16:39:58 incubator-aurora $ ./pants ./src/test/python/apache/aurora/admin:all
> Build operating on targets: 
> OrderedSet([PythonTestSuite(src/test/python/apache/aurora/admin/BUILD:all)])
> 
>  test session starts 
> =
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 9 items 
> 
> src/test/python/apache/aurora/admin/test_host_maintenance.py .
> 
> ==
>  9 passed in 0.67 seconds 
> ==
> src.test.python.apache.aurora.admin.host_maintenance  
>   .   SUCCESS
> 
> 
> Thanks,
> 
> Joe Smith
> 
>

Re: Review Request 21273: Add a "config" noun with a "list" verb to list jobs defined in a config file.

[Mark Chu-Carroll]

> On May 13, 2014, 12:46 p.m., David McLaughlin wrote:
> > src/main/python/apache/aurora/client/cli/config.py, lines 21-23
> > 
> >
> > Is it just me or a lot of these imports unused?

No, you're absolutely right. I started the file with a cut-and-paste, and then 
didn't go back and clean up the imports. Doing it now.


- Mark


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21273/#review42836
---


On May 9, 2014, 2:05 p.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21273/
> ---
> 
> (Updated May 9, 2014, 2:05 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-403
> https://issues.apache.org/jira/browse/aurora-403
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a "config" noun with a "list" verb to list jobs defined in a config file.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/BUILD 
> 0c5a8c5cd11ad29b43027c0556710018b6ee3adc 
>   src/main/python/apache/aurora/client/cli/client.py 
> f7bafca8285ba5779ee185273d4843995a241b70 
>   src/main/python/apache/aurora/client/cli/config.py PRE-CREATION 
>   src/test/python/apache/aurora/client/cli/BUILD 
> 9766b3bdd0f5f552349453b6724573d43ddb02e5 
>   src/test/python/apache/aurora/client/cli/test_config_noun.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21273/diff/
> 
> 
> Testing
> ---
> 
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_bridge.py 
> 
> === 4 passed in 0.04 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 9 items
> 
> src/test/python/apache/aurora/client/cli/test_command_hooks.py .
> 
> === 9 passed in 0.80 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_help.py .
> 
> === 5 passed in 0.65 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 37 items
> 
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py 
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ..
> src/test/python/apache/aurora/client/cli/test_open.py .
> src/test/python/apache/aurora/client/cli/test_restart.py ...
> src/test/python/apache/aurora/client/cli/test_status.py ...
> src/test/python/apache/aurora/client/cli/test_update.py ...
> 
> == 37 passed in 2.10 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_config_noun.py ...
> 
> === 3 passed in 0.66 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 1 items
> 
> src/test/python/apache/aurora/client/cli/test_logging.py .
> 
> === 1 passed in 0.67 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_plugins.py ...
> 
> === 3 passed in 0.73 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_quota.py 
> 
> === 4 passed in 0.88 seconds 
> ===
> = 

Re: Review Request 21205: Bugfix: restart doesn't notify user about invalid max_total_failures option.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21205/#review42832
---


Suman, ping?

- Mark Chu-Carroll


On May 8, 2014, 10:26 a.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21205/
> ---
> 
> (Updated May 8, 2014, 10:26 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-399
> https://issues.apache.org/jira/browse/aurora-399
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Bugfix: restart doesn't notify user about invalid max_total_failures option.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/jobs.py 
> cf45640a66c76ae4daa0ceaa3ff8da80c4cbe91d 
>   src/main/python/apache/aurora/client/commands/core.py 
> 0046c7686261ec11c2064348e21a35758b449d27 
>   src/test/python/apache/aurora/client/cli/test_restart.py 
> 9b7a2c69b53d4be35587f630f429943a4907b74b 
>   src/test/python/apache/aurora/client/commands/test_restart.py 
> 9af6334264f40cbccbef545bd15ffa85171ad348 
> 
> Diff: https://reviews.apache.org/r/21205/diff/
> 
> 
> Testing
> ---
> 
> [sun-wukong incubator-aurora (restart_max)]$ ./pants 
> src/test/python/apache/aurora/client:all
> Build operating on targets: 
> OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/BUILD:all)])
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 2 items
> 
> src/test/python/apache/aurora/client/test_binding_helper.py ..
> 
> === 2 passed in 0.36 seconds 
> 
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 6 items
> 
> src/test/python/apache/aurora/client/test_config.py ..
> 
> === 6 passed in 0.45 seconds 
> 
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 6 items
> 
> src/test/python/apache/aurora/client/api/test_disambiguator.py ..
> 
> === 6 passed in 0.41 seconds 
> 
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/api/test_job_monitor.py 
> 
> === 4 passed in 0.39 seconds 
> 
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 6 items
> 
> src/test/python/apache/aurora/client/api/test_restarter.py ..
> 
> === 6 passed in 0.34 seconds 
> 
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 47 items / 1 skipped
> 
> src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> ...
> 
> = 47 passed, 1 skipped in 4.05 seconds 
> ==
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 12 items
> 
> src/test/python/apache/aurora/client/api/test_instance_watcher.py 
> src/test/python/apache/aurora/client/api/test_health_check.py 
> 
> === 12 passed in 0.22 seconds 
> ===
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 28 items
> 
> src/test/python/apache/aurora/client/api/test_updater.py 
> 
> 
> === 28 passed in 0.67 seconds 
> ===
> == test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 6 items
> 
>

Re: Review Request 21352: Fix regression causing scheduling rate limiter to not be honored.

[Maxim Khutornenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21352/#review42834
---

Ship it!


Ship It!

- Maxim Khutornenko


On May 13, 2014, 12:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21352/
> ---
> 
> (Updated May 13, 2014, 12:06 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-416
> https://issues.apache.org/jira/browse/AURORA-416
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Since this would have been caught by findbugs when introduced, i decided to 
> configure findbugs on our build as added prevention going forward.
> 
> I also addressed a few findbugs warnings which took fewer characters to 
> exclude than to fix.
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   config/findbugs/excludeFilter.xml PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 
> ada5eafdf484f733c07277754833313d5e7bdc4b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> ddbb02560bb8fb94dafe26c1a66c767e9a3a863b 
>   src/test/java/org/apache/aurora/scheduler/async/TaskGroupsTest.java 
> e23ab5cd49a52f3004baa4e30462c6b028931371 
>   src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java 
> a9b85d0983dcfee89101a5e774ba86ee11708c68 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java 
> de5cdcf7a16f71f1815b0f6de6100eaf42c76cdd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  47d2fd6d8a34cb14d68894e14c147709373e3572 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> e212174ed089fdcf28fa679318fe216917a40b99 
> 
> Diff: https://reviews.apache.org/r/21352/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 21273: Add a "config" noun with a "list" verb to list jobs defined in a config file.

[Mark Chu-Carroll]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21273/#review42833
---


ping? (just trying to get this back into the mail queue.)

- Mark Chu-Carroll


On May 9, 2014, 2:05 p.m., Mark Chu-Carroll wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21273/
> ---
> 
> (Updated May 9, 2014, 2:05 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Suman Karumuri.
> 
> 
> Bugs: aurora-403
> https://issues.apache.org/jira/browse/aurora-403
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a "config" noun with a "list" verb to list jobs defined in a config file.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/BUILD 
> 0c5a8c5cd11ad29b43027c0556710018b6ee3adc 
>   src/main/python/apache/aurora/client/cli/client.py 
> f7bafca8285ba5779ee185273d4843995a241b70 
>   src/main/python/apache/aurora/client/cli/config.py PRE-CREATION 
>   src/test/python/apache/aurora/client/cli/BUILD 
> 9766b3bdd0f5f552349453b6724573d43ddb02e5 
>   src/test/python/apache/aurora/client/cli/test_config_noun.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/21273/diff/
> 
> 
> Testing
> ---
> 
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_bridge.py 
> 
> === 4 passed in 0.04 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 9 items
> 
> src/test/python/apache/aurora/client/cli/test_command_hooks.py .
> 
> === 9 passed in 0.80 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_help.py .
> 
> === 5 passed in 0.65 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 37 items
> 
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py 
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py ..
> src/test/python/apache/aurora/client/cli/test_open.py .
> src/test/python/apache/aurora/client/cli/test_restart.py ...
> src/test/python/apache/aurora/client/cli/test_status.py ...
> src/test/python/apache/aurora/client/cli/test_update.py ...
> 
> == 37 passed in 2.10 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_config_noun.py ...
> 
> === 3 passed in 0.66 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 1 items
> 
> src/test/python/apache/aurora/client/cli/test_logging.py .
> 
> === 1 passed in 0.67 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 3 items
> 
> src/test/python/apache/aurora/client/cli/test_plugins.py ...
> 
> === 3 passed in 0.73 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_quota.py 
> 
> === 4 passed in 0.88 seconds 
> ===
> = test session starts 
> ==
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> plugins: cov
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_sla.py .
> 
> === 5 passed in 0.72 seconds 
> ===
> ==

Re: Review Request 21247: Add config grouping visualisation to job page

[David McLaughlin]

> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > I think we should show the config bar even when there is one config. It 
> > will also act a visual indication to the user that all his tasks are in a 
> > consistent config.

This is a fair suggestion, and I tried this initially. Having a 100% color bar 
across the header of the page dominates the user's attention when there is 
nothing for them to see. 


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > File Attachment: Viewing multiple configs at the same time - Screen Shot 
> > 2014-05-08 at 4.25.42 PM.png
> > 
> >
> > What happens when there are more than 2 configs? Will these configs 
> > wrap around?
> > 
> > please add a close option to divs so the users can hide the configs 
> > they don't want. That will let users compare the different configs all at 
> > once.

Yes, see the CSS. It is a fluid floating list which will scale with the page 
width. If you look at the code, the bars are toggles to show/hide individual 
configs. 


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html, 
> > line 1
> > 
> >
> > wrap this in a div.

Why?


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 387
> > 
> >
> > please move all the code related to group summary into it's own 
> > controller. That will make the code modular and easily testable.

I think it's best to keep the code consistent with what is there already, and 
fix the design of the app in another ticket. 


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 385
> > 
> >
> > please only pass the required fields into the fields into the function. 
> > By passing in $scope, it is very unclear what inputs buildGroupSummary 
> > depends on.

The function signature is quite clear that buildGroupSummary is a function 
which depends on the global $scope (it both reads and writes to it - this is 
how Angular passes around such objects to its controllers which do the same 
thing). 


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 396
> > 
> >
> > Please initialize all the new fields to scope outside this function. As 
> > is, a typo in a variable name will cause the UI to render incorrectly and 
> > it would be really hard to debug what went wrong.

The function is always called, there is no need to initialize the fields 
outside of the function. 

Typos in variable names are always a source of bugs and whether the typo is in 
the parent scope or this one doesn't really change that. 


- David


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21247/#review42626
---


On May 8, 2014, 11:45 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> ---
> 
> (Updated May 8, 2014, 11:45 p.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Bugs: AURORA-378
> https://issues.apache.org/jira/browse/AURORA-378
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Replace the button to show/hide configs with a bar when there are multiple. 
> Tidied up the display of the configuration data. 
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 983101277ffbd1c4017b29f4c86e61315f1bcc78 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/configSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> dc515ac818a9af36522bb07a125cd92ff9230fa2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/groupSummary.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> cd78c20bee7891a6cdfd19943a6f7e8a9dce33df 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7c80bd769b7e80bef0822846166959925b1bf7f8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> a79276d2486fc1122a8b084ea0614cdae759f88c 
> 
> Diff: https://reviews.apache.org/r/21247/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Job page with onl