aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David McLaughlin" <da...@dmclaughlin.com>
Subject Re: Review Request 21247: Add config grouping visualisation to job page
Date Wed, 14 May 2014 15:48:22 GMT


> 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
> > <https://reviews.apache.org/r/21247/diff/5/?file=581259#file581259line386>
> >
> >     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.
> 
> David McLaughlin wrote:
>     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 <anonymous> (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) <ng-view
class="ng-scope">
> 
> Suman Karumuri wrote:
>     The code is breaks the linear control flow for no good reason. I fail to understand
the benefit of doing it the way you did it. The fact that you added a comment indicates a
code smell. 
>     
>     If it helps, please rewrite getTasksForJob as follows: 
>     
>         $scope.activeTasks = [];
>         
>         getTasksForJob($scope, auroraClient);
>     
>         function getTasksForJob($scope, auroraClient) {
>           var response = auroraClient.getTasks($scope.role, $scope.environment, $scope.job);
>     
>           if (response.error) {
>             $scope.error = 'Error fetching tasks: ' + response.error;
>             return [];
>           }
>     
>           $scope.jobDashboardUrl = getJobDashboardUrl(response.statsUrlPrefix);
>     
>           $scope.taskSummary = taskUtil.summarizeActiveTaskConfigs(response.tasks);
>           buildGroupSummary($scope);
>           
>           var tasks = _.map(response.tasks, function (task) {
>             return summarizeTask(task);
>           });
>     
>           var activeTaskPredicate = function (task) {
>             return task.isActive;
>           };
>     
>           $scope.completedTasks = _.chain(tasks)
>             .reject(activeTaskPredicate)
>             .sortBy(function (task) {
>               return -task.latestActivity; //sort in descending order
>             })
>             .value();
>     
>     
>           $scope.activeTasks = _.chain(tasks)
>             .filter(activeTaskPredicate)
>             .sortBy(function (task) {
>               return task.instanceId;
>             })
>             .value();
>         }
>     
>     This is just a 4 line change and gets rid of the side-effects you don't like. Further
refactoring can be performed in future reviews.

It's linear either way, but I'm conceding defeat here. Moved buildGroupSummary into that function.



- David


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


On May 14, 2014, 3:46 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21247/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 3:46 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 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message