aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua Cohen" <jco...@twopensource.com>
Subject Re: Review Request 25259: Add update information to the scheduler UI
Date Fri, 12 Sep 2014 21:20:07 GMT

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


Mostly just nitpicky style/readability stuff...


src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js
<https://reviews.apache.org/r/25259/#comment92717>

    This might read a little bit cleaner if you chained it all?
    
        return instanceActionLookup[action] || 'UNKNOWN'
          .replace(/INSTANCE_/, '')
          .replace(/_/g, ' ');



src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
<https://reviews.apache.org/r/25259/#comment92719>

    I'm unclear on the need to convert these arrays of statuses above to objects just to check
for the presence of a value? Is there a reason we can't simply use indexOf on the array and
save the transform?



src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
<https://reviews.apache.org/r/25259/#comment92723>

    just inline instanceActionLookup[action] here?



src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
<https://reviews.apache.org/r/25259/#comment92725>

    given the similarity to the reverse events iteration in `progressFromEvents` above it
might be worthwhile to extract a function like (but with a better name...):
    
        function reverseEventsFilter(instanceEvents, condition) {
          var events = _.sortBy(instanceEvents, 'timestampMs');
          var instanceMap = {};
          condition = condition || function() {};
          for (var i = events.length - 1; i >= 0; i++) {
            if (instanceMap.hasOwnProperty(events[i].instanceId) && condition(event))
{
              instanceMap[event.instanceId] = event;
            }
          }
          
          return instanceMap;
        }
        
    Then the logic here just becomes:
    
        var instanceMap = reverseEventsFilter(details.instanceEvents);
        
    And the logic above in `progressFromEvents` becomes something like:
    
        return Object.keys(reverseEventsFilter(instanceEvents, function(e) { updateUtil.isInstanceSuccessful(e.action);
})).length;



src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
<https://reviews.apache.org/r/25259/#comment92727>

    kill blank line.



src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
<https://reviews.apache.org/r/25259/#comment92726>

    isSubset checks to make sure subset is truthy, and since it's iterating to subset.length
we can probably skip both of these checks and just make this `if (inSubset(i))`
    
    Also, how do you feel about passing subset in as a param to isSubset instead of picking
it up via a closure?


- Joshua Cohen


On Sept. 9, 2014, 11:50 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25259/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2014, 11:50 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Bill Farner.
> 
> 
> Bugs: AURORA-614
>     https://issues.apache.org/jira/browse/AURORA-614
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adds update history to the job page. Adds an update details page. 
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java de49a1c5497e32ee4db944412e5c87807c742d3c

>   src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html c780b0fe98863b5421824a9652a7202da9f4302a

>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 2a752313cb8ae404605a9458b33237a911eec061

>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html e21dee015897eee62ade8f74e26f042b8e2be734

>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js fb3b5b12121a6e8a30dbf6fe069557f69a563408

>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 3477b7e667459665af9d9dc9d2456793822bc7f7

>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 7f05a552f3786adb115ff9648f4fadce968230e9

>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js df2806481fc1c2f263d3afd9b21247e97a18ed57

>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js bfd360de45c75441743c8ba24a5c445f4146dba6

>   src/main/resources/org/apache/aurora/scheduler/http/ui/timeDisplay.html PRE-CREATION

>   src/main/resources/org/apache/aurora/scheduler/http/ui/update.html PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/updateSettings.html PRE-CREATION

>   src/main/thrift/org/apache/aurora/gen/api.thrift 2b376d663b3bc9264965db58ef89de59b10169ad

> 
> Diff: https://reviews.apache.org/r/25259/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew jsHint
> 
> 
> File Attachments
> ----------------
> 
> job page
>   https://reviews.apache.org/media/uploaded/files/2014/09/09/531eca81-a0ba-4438-8bd6-4b50d97b0270__job-progress-small-preview.png
> update page
>   https://reviews.apache.org/media/uploaded/files/2014/09/09/8e3f2950-7d7e-404e-bca9-6c472b5218f7__update-page-finished.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


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