> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > A few general Javascript comments: there seems to have been an explosion of 
> > global state and some controllers are getting pretty big. Consider using 
> > angular's dependency injection system to keep dependencies explicit and 
> > managed rather than relying on global bindings.
> > 
> > There seems to be a general lack of documentation for utility functions and 
> > controllers. Consider factoring common utility functions into their own 
> > documented services and injecting them.
> > 
> > The controllers code has grown several hundred lines but there's a lack of 
> > tests - it may be time to bite the bullet and write some. In theory Angular 
> > makes this easy, but I haven't done it before.

JobController is a weird controller in that respect since all the data on the 
page is seeded by a single call to the scheduler backend. 
Refactored some of the task config summary logic into a service as a 
refactoring step. Ideally we shouldn't be doing it since the service is used 
only by one controller. 

Added documentation.

Tests are out of scope for this change. We will add them going forward.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 75
> > <https://reviews.apache.org/r/19833/diff/6/?file=549444#file549444line75>
> >
> >     Url

Fixed.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > build.gradle, line 343
> > <https://reviews.apache.org/r/19833/diff/6/?file=549443#file549443line343>
> >
> >     Any reason this can't be dropped?

The application will default it to empty string. But, having a fake URL will 
help us when testing the local UI. This is an optional argument already.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines 
> > 134-136
> > <https://reviews.apache.org/r/19833/diff/6/?file=549445#file549445line134>
> >
> >     Since this is being passed to AppModule move it there. Convention is to 
> > have modules that care about a flag define them there.

All the arguments that are passed into AppModule are defined in this way. 
Leaving it as is for consistency for now.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 270
> > <https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line270>
> >
> >     Is there a reason why you don't use call-chaining here? Most of the 
> > examples I've seen use it and I find it more readable, especially in a 
> > dynamic language where a typo in "auroraUIControllers" is a runtime error.

The angularJS style guide actually recommends against chaining 
https://docs.google.com/a/twitter.com/document/d/1XXMvReO8-Awi1EZXAXS4PzDzdNvV6pGcuaF4Q9821Es/pub.
 The reference phone cat app is here: 
https://github.com/angular/angular-phonecat/blob/master/app/js/controllers.js

Chaining for large controllers makes code unreadable and makes the code very 
inflexible. A typo is not an issue since the router catches an error upon 
application startup on a missing controller.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > lines 296-304
> > <https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line296>
> >
> >     Push this down to the template with ng-hide="showSummary"?

Moving the logic into a template makes this logic untestable. So, leaving it in 
the controller.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 328
> > <https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line328>
> >
> >     Inject this?

This function is called by smart table while it is rendering it's data sort of 
as a filter. It doesn't have access to any AngularJs variables at that point 
so, can't inject this. 


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 359
> > <https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line359>
> >
> >     prefer lowcasing abbreviations - URL => Url

Done.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 365
> > <https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line365>
> >
> >     This both mutates the current scope and pretends to be a function - why 
> > not make it void or return {active: [], completed: []}

Can't make it return an object since we need AngularJS bindings to work. Can't 
make it void since we can't bind to the scope variable being initialized. The 
only way here is to use a superfluous scope variable which makes the code less 
DRY. There is no clean solution here. A book I read on the topic recommends it 
be done this way.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 30
> > <https://reviews.apache.org/r/19833/diff/6/?file=549477#file549477line30>
> >
> >     These should be documented.

Done.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 63
> > <https://reviews.apache.org/r/19833/diff/6/?file=549477#file549477line63>
> >
> >     statsUrlPrefix
> >     
> >     Also consider making this nullable and hiding the icon behind an 
> > ng-hide if it's null.

Renamed. 

We already hide the icon gated by the jobDashboardUrl.


> On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 439
> > <https://reviews.apache.org/r/19833/diff/6/?file=549483#file549483line439>
> >
> >     statsUrlPrefix. Also, is there any reason this can't be optional - 
> > seems like null should allow the scheduler to continue operating fine 
> > without needing a fake value.

Renamed to statsUrlPrefix. I see making this field optional and setting it to 
null as orthogonal issues. This field shouldn't be optional since we pass 
serverInfo with every call. This field need to be set to start the scheduler, 
it can initialized to an empty string already. 


- Suman


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


On April 4, 2014, 11:52 p.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19833/
> -----------------------------------------------------------
> 
> (Updated April 4, 2014, 11:52 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-281
>     https://issues.apache.org/jira/browse/AURORA-281
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. 
> Added it here so only new code can be reviewed. ***
> 
> 
> Migrated job page to AngularJS.
> Removed old scheduler job page.
> Removed js related to the old scheduler page. Removed data tables.
> Added moment.js library for date time manipulation.
> Refactored code a bit for reusability.
> Stats link should still be added to the new jobs page.
> 
> Since the role/env code is not committed, this diff contains that code as 
> well. 
> Code changes start from page 4 in diff between version 1 and 2.
> 
> 
> Diffs
> -----
> 
>   build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> eeafc784e915137cacd5f64df1252ccbaf6c0f6c 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> ec56c649116c03ef148bac916bd6691a94685bc3 
>   src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 
> 19df7889f15b4cf44e386d8ce0626cc94fdcdfba 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 
> 2ccc6f367b9715a0abb3e0673069289ae4860087 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> e3ff2571d95effcf72b2047cc5840d56143a180c 
>   src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java 
> b35aad8db006133ba70692e43db1f083d4950914 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css
>  ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png
>  881de7976ff98955e2a5487dca66e618a0655f3d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png
>  c608682b04a6d9b8002602450c8ef7e80ebba099 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png
>  d300f1064b3beac1d7d5274e294494d3143e53a2 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png
>  6a6ded7de821619aedc71d1738c0b73463a4452e 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png
>  a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png
>  fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png
>  a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png
>  4e144cf0b1f786a9248a2998311e8109998d8a2d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png
>  18670406bc01ab2721781822dd6478917745ff54 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png
>  def071ed5afd264a036f6d9e75856366fd6ad153 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png
>  7824973cc60fc1841b16f2cb39323cefcdc3f942 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js
>  420e507086f7a2af08f68787f67208546745260d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js
>  885ad347f3076dcf75558ef44806dc25a8171884 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js
>  3c0b39aefbd5a82adee49f2ac7539b6974576b2f 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js
>  02694a4a56eb247baa2398d971927dfbd1ac3e60 
>   src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 
> 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 
>   src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 
> 5dd0d17849c7116d70925275b9c6c0715b441f14 
>   src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 
> 28b56671b2e825912a6427e609c2bbe1e7758e26 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> ade850ce624964693e9bd55946464983c4b9f8c2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
> 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js 
> db6ea99aeb749fd8674613e3620dc3012872e13c 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7cd534479dd2f17ffd46248ce9af1f8fe89beb97 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> d2b2017a0efc70d425fd6c89ad6caaf46cb8ded5 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> 8681bbe840b6285b15dd6766016258c2c8115632 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/role.html 
> PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/http/ui/schedulingDetail.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskLink.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskSandbox.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskStatus.html 
> PRE-CREATION 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> c0618e4edebd6f282698abfd9bdc3c36fff16920 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> a4e9464f7d5d3f5a640b62557c3e29f2f1566985 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  fae2de11235dd059718e1023fdcfb0e8fc4deadd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  dd991fb90889c3f221e537a78283eb4a31e5f0dd 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 05c6e8a1e2407f2822b3844c555d8995f1cd1d49 
> 
> Diff: https://reviews.apache.org/r/19833/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew clean build run on local laptop.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to