> On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 253 > > <https://reviews.apache.org/r/30890/diff/1/?file=861038#file861038line253> > > > > s/SchedulingFilter.//
Done. > On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 34 > > <https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line34> > > > > I feel like `VetoType` still applies and is actually a better name. > > It's odd for an enum value to be considered a 'group'. (When i hear group > > i think collection.) VetoType is used to describe the particular type of veto applied. This enum denotes similar typed Veto groups. I am fine changing it to something else if VetoGroup is confusing. Any suggestions? > On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 174 > > <https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line174> > > > > Is this comment useful? Ignoring the performance requirements, it > > seems like a perfectly rational implementation. >Ignoring the performance requirements, it seems like a perfectly rational >implementation. It's an attempt to keep it as such (e.g. avoiding Optional and the like). > On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 274 > > <https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line274> > > > > Revert to former location? Seems like noise in the diff Split/merge side-effect. Fixed. > On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, > > line 115 > > <https://reviews.apache.org/r/30890/diff/1/?file=861040#file861040line115> > > > > What's the thought process behind converting this to a `Set`? The code > > doesn't create duplicates, and you pick up the performance hit of a hash > > table. > > > > I'm actually wondering if signatures should use `Iterable` all the way > > out of this class. The only consumer of this method is already converting everything to a set. It feels awkward to have Iterable in places where there should be no duplicates to start with. I am -1 on converting all signatures from Set to Iterable in this class. The interface contract should clearly state the expectations. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30890/#review72061 ----------------------------------------------------------- On Feb. 11, 2015, 9:38 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30890/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2015, 9:38 p.m.) > > > Review request for Aurora, Kevin Sweeney and Bill Farner. > > > Bugs: AURORA-909 > https://issues.apache.org/jira/browse/AURORA-909 > > > Repository: aurora > > > Description > ------- > > Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and > refactoring TaskVars. > > Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a > parent. > > Original RB: https://reviews.apache.org/r/28617/ > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/TaskVars.java > f017cdd26ca40138a7e141f21613ed567314c399 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > f06fdaeb92e154d0982bdabed5df93e7bcba9048 > src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java > 4e7efb3c1214c3d193afd61f162713490eb8effb > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > 52ee7c1e3742d9315c7e7aaa77677121e1e9288d > > Diff: https://reviews.apache.org/r/30890/diff/ > > > Testing > ------- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
