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