Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============5265678106669718589=="
MIME-Version: 1.0
Subject: Re: Review Request 21247: Add config grouping visualisation to job
 page
From: "David McLaughlin" <david@dmclaughlin.com>
To: "Bill Farner" <wfarner@apache.org>, "Suman Karumuri" <mansu@apache.org>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "David McLaughlin" <david@dmclaughlin.com>
Date: Wed, 14 May 2014 15:48:22 -0000
Message-ID: <20140514154822.4187.67230@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "David McLaughlin" <noreply@reviews.apache.org>
References: <20140514042641.4080.39328@reviews.apache.org>
In-Reply-To: <20140514042641.4080.39328@reviews.apache.org>
Reply-To: "David McLaughlin" <david@dmclaughlin.com>

--===============5265678106669718589==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> 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
> 
>


--===============5265678106669718589==--