> 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.) > > Maxim Khutornenko wrote: > 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?
My mistake, the diff convinced me that `VetoType` was renamed, but i see that it still exists. How about `VetoReason` and s/`EMPTY`/`NONE`/? Reads a bit more naturally from the calling side, but i don't feel strongly on that. > 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. > > Maxim Khutornenko wrote: > 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. I generally agree that `Set` should be preferred to communicate uniqueness to callers. However, in the same vein as the performance sensitivity advised above, constructing a hash table non-negligible performance hit. In case i was unclear, i suggest you change the `SchedulingFilter` signature as well, and avoid ever turning the result into a `Set`. How would you feel about peeling off a ticket to explore this? As i follow the result, it looks like it could ripple fairly wide and distract from this review. I might have a chance to pick this up next sprint. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30890/#review72061 ----------------------------------------------------------- On Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30890/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2015, 6:05 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 > >
