----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review40443 -----------------------------------------------------------
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. build.gradle <https://reviews.apache.org/r/19833/#comment73450> Any reason this can't be dropped? src/main/java/org/apache/aurora/scheduler/app/AppModule.java <https://reviews.apache.org/r/19833/#comment73448> Url src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java <https://reviews.apache.org/r/19833/#comment73451> Since this is being passed to AppModule move it there. Convention is to have modules that care about a flag define them there. src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java <https://reviews.apache.org/r/19833/#comment73452> Push this down to AppModule src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java <https://reviews.apache.org/r/19833/#comment73453> Push this down to AppModule src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73457> 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. src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73464> Push this down to the template with ng-hide="showSummary"? src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73465> Inject this? src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73469> prefer lowcasing abbreviations - URL => Url src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js <https://reviews.apache.org/r/19833/#comment73472> This both mutates the current scope and pretends to be a function - why not make it void or return {active: [], completed: []} src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73481> These should be documented. src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js <https://reviews.apache.org/r/19833/#comment73480> statsUrlPrefix Also consider making this nullable and hiding the icon behind an ng-hide if it's null. src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/19833/#comment73479> 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. src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java <https://reviews.apache.org/r/19833/#comment73483> Again push this into AppModule. - Kevin Sweeney On April 4, 2014, 4: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, 4: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 > >
