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 25259: Add update information to the scheduler UI
Date Fri, 12 Sep 2014 22:32:01 GMT


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js, lines 69-72
> > <https://reviews.apache.org/r/25259/diff/3/?file=684205#file684205line69>
> >
> >     This might read a little bit cleaner if you chained it all?
> >     
> >         return instanceActionLookup[action] || 'UNKNOWN'
> >           .replace(/INSTANCE_/, '')
> >           .replace(/_/g, ' ');

Fixed.


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 193
> > <https://reviews.apache.org/r/25259/diff/3/?file=684206#file684206line193>
> >
> >     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?

It's definitely not worth it for status updates since even O(nm) is going to be trivially
small, but I think it's worthwhile* for the instance actions since you're talking about potentially
thousands of actions there. And once you've got that toSet function, it's basically the same
amount of code either way. 


* Don't have a single benchmark to back this up, probably premature on something like v8.


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 246
> > <https://reviews.apache.org/r/25259/diff/3/?file=684206#file684206line246>
> >
> >     just inline instanceActionLookup[action] here?

Fixed.


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, lines 252-256
> > <https://reviews.apache.org/r/25259/diff/3/?file=684206#file684206line252>
> >
> >     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;

Yeah, I had repeated this code at least one more time when I had the event timeline viz too.
I like this solution. 

Fixed.


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 268
> > <https://reviews.apache.org/r/25259/diff/3/?file=684206#file684206line268>
> >
> >     kill blank line.

Fixed.


> On Sept. 12, 2014, 9:20 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 289
> > <https://reviews.apache.org/r/25259/diff/3/?file=684206#file684206line289>
> >
> >     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?

+1 for making subset a param and removing the length check. 

But the other suggestion breaks the logic. It really is "if they have declared a subset AND
this instance id isn't in that subset.. it's not going to be part of the update" (i.e. ignored).
You can rejig the else if and else but you always have to check both conditions. I thought
you were right at first, which speaks to the clarity of the code here...  so I added a comment
to clarify. (And also speaks for how badly we need unit tests!)


- David


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


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