> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/ServletModule.java, line 225
> > <https://reviews.apache.org/r/19565/diff/1/?file=533620#file533620line225>
> >
> >     This method makes for difficult-to-read call-sites.  How about 
> > silent=true for everything to remove the extra flag?  You can then add the 
> > /scheduler link to / with:
> >     
> >       Registration.registerEndpoint(binder(), "/scheduler");

Yeah, this is better. Changed.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html, 
> > line 5
> > <https://reviews.apache.org/r/19565/diff/1/?file=533622#file533622line5>
> >
> >     Consider removing the spaces on the LHS of colons:
> >     
> >       Role: myRole
> >       Environment: myEnv
> >     
> >     as opposed to
> >     
> >       Role : myRole
> >       Environment : myEnv

I wanted to remove them but wasn't sure if they were intentional so left them 
there. Removed now.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html, 
> > line 7
> > <https://reviews.apache.org/r/19565/diff/1/?file=533622#file533622line7>
> >
> >     Is the extra "if" redundant here?

yes. Removed.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/home.html, line 19
> > <https://reviews.apache.org/r/19565/diff/1/?file=533623#file533623line19>
> >
> >     columnCollection and rowCollection probably aren't the most informative 
> > names.  Can you be more descriptive?

Changed.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 56
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line56>
> >
> >     DRY

Refactored error handling logic into a directive for more dryness.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 81
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line81>
> >
> >     Why not:
> >     
> >     $scope.showResources = !$scope.showResources;
> >     $scope.resourceButtonText = ($scope.showResources ? 'Show' : 'Hide') + 
> > ' Resource Consumption';

Changed. Preferring separate strings for the button text separate since we can 
change the text in future without changing the logic or for i18n purposes. 


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 88
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line88>
> >
> >     missing semicolon

added


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 205
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line205>
> >
> >     Does it make sense to s/cronJobName/jobName/, then you can reuse the 
> > sort function? (This also results in more consistent naming.)
> >     
> >     You could take this a step further to declare a local function that 
> > produces the basic information for a job (role, environment, name), and 
> > decorate that with the type-specific fields.  Your call on that part.

Good idea. Changed. Not taking it a step further, since this code more simpler 
to understand. 


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 212
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line212>
> >
> >     missing semicolon

Fixed.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 215
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line215>
> >
> >     missing semicolon

fixed


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 231
> > <https://reviews.apache.org/r/19565/diff/1/?file=533627#file533627line231>
> >
> >     missing semicolon

fixed


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/role.html, line 2
> > <https://reviews.apache.org/r/19565/diff/1/?file=533630#file533630line2>
> >
> >     This is a copy-paste from home.html, and will probably drift.  Can you 
> > use a fragment instead?

Turned it into a directive.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/role.html, line 20
> > <https://reviews.apache.org/r/19565/diff/1/?file=533630#file533630line20>
> >
> >     funky indent?

Fixed.


> On March 24, 2014, 6:35 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/role.html, line 5
> > <https://reviews.apache.org/r/19565/diff/1/?file=533630#file533630line5>
> >
> >     Right now you have a 'global' scope with:
> >     
> >       title
> >       error
> >       reloadMsg
> >       errorMsg
> >       columnCollection
> >       role
> >       environment
> >       quotaError
> >       quotaErrorMsg
> >       jobsTableConfig
> >       jobsTableColumns
> >       cronJobsTableConfig
> >       cronJobsTableColumns
> >       showResources
> >       resourceButtonText
> >     
> >     (possibly more that i've missed)
> >     
> >     And some of these have relationships.  It would be nice to have 
> > relationships follow the scoping (i shouldn't have an errorMsg or reloadMsg 
> > without an error).  Laying out a scoping convention early will also help 
> > define the pattern before the scope becomes unwieldy.

The scope of all these variables is within the JobSummaryController and so not 
global. I have refactored errors into a directive to make this a bit more 
cleaner. I have also refactored quota code into a new controller so the 
JobSummaryController is more focussed. 


- Suman


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


On March 23, 2014, 4:35 a.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19565/
> -----------------------------------------------------------
> 
> (Updated March 23, 2014, 4:35 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-39
>     https://issues.apache.org/jira/browse/AURORA-39
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implemented the scheduler role and role/env pages using AngularJS.
> 
> Added angular route and underscore.js modules.
> 
> 
> Diffs
> -----
> 
>   3rdparty/javascript/bower_components/angular-route/.bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/README.md PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.js 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.min.js 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/angular-route.min.js.map 
> PRE-CREATION 
>   3rdparty/javascript/bower_components/angular-route/bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.editorconfig PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/.gitignore PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/LICENSE PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/README.md PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/bower.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/component.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/package.json PRE-CREATION 
>   3rdparty/javascript/bower_components/underscore/underscore.js PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 
> 19df7889f15b4cf44e386d8ce0626cc94fdcdfba 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 
> e2f9ed0ea846c570de11b7dd85bc90aee6bc3342 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> e3ff2571d95effcf72b2047cc5840d56143a180c 
>   src/main/resources/org/apache/aurora/scheduler/http/schedulerzrole.st 
> b53f3524be052dcd5882c3e79e95e2f90aa071b8 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/home.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
> 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/jobLink.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/services.js 
> 81cd12c4fea473192cd7e6b6dba245e4dde30b3d 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/role.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/roleEnvLink.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/roleLink.html 
> fc25526389749e95efa87c719062dcea88935383 
> 
> Diff: https://reviews.apache.org/r/19565/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew clean build run on local laptop.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to