aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 21247: Add config grouping visualisation to job page
Date Tue, 13 May 2014 17:52:49 GMT


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > I think we should show the config bar even when there is one config. It will also
act a visual indication to the user that all his tasks are in a consistent config.
> 
> David McLaughlin wrote:
>     This is a fair suggestion, and I tried this initially. Having a 100% color bar across
the header of the page dominates the user's attention when there is nothing for them to see.
> 
> Suman Karumuri wrote:
>     I think the bar grabbing the user's attention when he goes to a page is ok in this
case because we are telling him that all the jobs are having the same config. Once we add
something else like a job status bar that needs user's attention, we can consider hiding this
automatically. Further, when we add auto-refresh to the page, the bar would keep appearing
and disappearing during an update which can be jarring. So, I think we should show the bar
always.
> 
> David McLaughlin wrote:
>     Well, there is no need for a visualisation when everything is homogenous. You just
need one link to show or hide the config, similar to what we have right now. The purpose of
the visualisation is to grab the user's attention and let them know - hey this job has special
instances that you may not know about (failed updates, canaries, etc.). Always showing the
bar is sort of like an 'everything is ok alarm'.
>     
>     The auto-refresh use case - in practice it would only appear when the update is in
progress and disappear when it is complete. If it is jarring then it is so by design ("hey!
look at this! something interesting is happening!"). It will not flicker during the update
itself. 
>     
>     I'll let someone else tie-break this though. Remember that this is just an initial
design, when we add more features to this page and receive user feedback, we will adapt to
that.

I'm in favor of giving the person behind the keyboard the majority vote here.  Sounds like
David has played with both approaches and found the current diff most user-friendly.  Let's
re-assess if we find otherwise.


> On May 10, 2014, 2:14 a.m., Suman Karumuri wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 385
> > <https://reviews.apache.org/r/21247/diff/1/?file=577123#file577123line385>
> >
> >     please only pass the required fields into the fields into the function. By passing
in $scope, it is very unclear what inputs buildGroupSummary depends on.
> 
> David McLaughlin wrote:
>     The function signature is quite clear that buildGroupSummary is a function which
depends on the global $scope (it both reads and writes to it - this is how Angular passes
around such objects to its controllers which do the same thing).
> 
> David McLaughlin wrote:
>     Just to be clear on this point. Take a look at getTasksForJob:
>     
>         function getTasksForJob(role, environment, job) {
>            ...
>         }
>     
>     You might think this gives some form of clarity, but it does not. It's confusing,
because this function goes on to use the following variables via a closure:
>     
>      auroraClient, $scope, taskUtil, getJobDashboardUrl, summarizeTask
>     
>     
>     On the other hand, everything that buildGroupSummary needs is passed into the function
or created inside it. You could copy and paste this function elsewhere in the code base -
move it completely outside of the controller or even pass it into the controller via DI and
the call to this function _from the controller_ would not change. I think ultimately all these
functions used to group code together need to be moved and this controller simplified, but
I want to keep this review small and do the larger cleaning up in another ticket.
> 
> Suman Karumuri wrote:
>     Ok. we can discuss the cleanup and code style else where.

Mind having this discussion in the abstract on the dev mailing list?  As a javascript noob,
I find it interesting, and wouldn't mind following along.


- Bill


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


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