> On May 10, 2016, 12:37 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, > > line 80 > > <https://reviews.apache.org/r/47050/diff/2/?file=1375040#file1375040line80> > > > > Building on my feedback on the review that introduced ResourceBag, do > > you think it'd make sense to have a helper method on that class like > > `streamResourceVectors()` to say on the redundant > > `getResourceVectors().entrySet().stream()` that are popping up all over the > > place now? > > > > Or maybe just `Set<ResourceType> ResourceBag#getTypes()` which returns > > `getResourceVectors().getEntrySet()`?
I am generally trying to avoid callsite optimizations until the end of refactoring but the `.entrySet().stream()` pattern appears to be quite dominant to suggest a helper method. Done. > On May 10, 2016, 12:37 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java, > > lines 117-128 > > <https://reviews.apache.org/r/47050/diff/2/?file=1375043#file1375043line117> > > > > Any reason we still create an anonymous `Function` impl, rather than > > just using a lambda here? Unfortunately, java lambdas are not treated equal to anonymous classes (http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.27): > Unlike code appearing in anonymous class declarations, the meaning of names > and the this and super keywords appearing in a lambda body, along with the > accessibility of referenced declarations, are the same as in the surrounding > context (except that lambda parameters introduce new names). What that vaguely articulated "surrounding context" statement really means is that constructor assignment precedence is uknown to lambdas. So, converting this to lambda would require making global `tierManager` and `executorSettings` non-final, which I'd rather avoid. > On May 10, 2016, 12:37 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java, > > lines 141-160 > > <https://reviews.apache.org/r/47050/diff/2/?file=1375043#file1375043line141> > > > > This seems relatively inefficient. Worst case scenario we'd be > > iterating the length of the list 4 times, where, in theory, we'd only need > > to do it once? Roughly, something like this should work? Or am I missing > > something? Could probably be rewritten to use `Stream#reduce` if desired. > > > > boolean hasGreater = hasLessThan = false; > > for (entry : left.resourceTypes()) { > > int comparison = > > entry.getValue().compareTo(right.valueOf(entry.getKey())); > > if (comparison < 0) { > > hasLessThan = true; > > } else if (comparison > 0) { > > hasGreater = true; > > } > > } > > > > return !hasGreater && !hasLessThan ? 0 : hasGreater ? 1 : 0; The above return statement does not account for -1 case and if amended would become unreadable. This is a direct translation of the existing behavior that I'd rather not change in this diff. Given the short-circuiting of the `.allMatch()` statement and the number of elements in the list that would likely never exceed single digits, I'd favor readability over optimization here. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47050/#review132357 ----------------------------------------------------------- On May 6, 2016, 8:22 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47050/ > ----------------------------------------------------------- > > (Updated May 6, 2016, 8:22 p.m.) > > > Review request for Aurora, Joshua Cohen and Bill Farner. > > > Repository: aurora > > > Description > ------- > > Migrating some `ResourceSlot` customers to `ResourceBag`. The selection here > is somewhat arbitrary to minimize the diff size. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > 501e6431f21822d9816952377546586da02ce42a > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > 1ee2cfa231d32d91b620dddd7aee6c1047424cb3 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > 6b5b12bce8052b93e921d060dd0e196b2554eefd > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > b325106c7f45b1ad1657221aaa39e3a428719ab0 > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictim.java > 98be997b0343de85d3c376a770d574437ede69a6 > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > 032ab2d4a4aa07ac78aba36149801cd545bd6f5b > src/main/java/org/apache/aurora/scheduler/quota/QuotaCheckResult.java > 99f034f58d2858863568f85177fc42753bcd6f17 > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java > 6d0d120e563b403f6fbe171a1ca23a84ff242e40 > src/main/java/org/apache/aurora/scheduler/resources/ResourceBag.java > 7916ec024b92b1900a50f004aefdca9ae9229031 > src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java > 69087e6ff140ee84e02731840be1b3dce2d9bb7e > src/main/java/org/apache/aurora/scheduler/resources/ResourceSlot.java > 13922bc650636ccd28cf60bcd893bce78e751306 > src/main/java/org/apache/aurora/scheduler/sla/SlaGroup.java > 21121bcf5f9f42ff60f86a1861aca2333bc9e99e > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > b6e43d798fd7d4de9e9a67a05f54c58b8663c95a > src/main/java/org/apache/aurora/scheduler/stats/AsyncStatsModule.java > 1d1415ac9ccddc607384fac59fa523b701346f3d > > src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java > 1474fa90eab3d018a499e8aefc7f9266e892f72a > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > 94a885fae8179f0f52bff6985fd9052b857f6b6f > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > ad397c6924b025f1eefb2bb02a6dc1e1f10ca078 > src/test/java/org/apache/aurora/scheduler/mesos/Offers.java > b266554b19a571f794edcc1aefa8571f1c406891 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 4efa696b6ee196837f775349a9b2b8f655fa4575 > src/test/java/org/apache/aurora/scheduler/resources/ResourceBagTest.java > 48724d5d75cc78555e98f29b2ed8ace9901ccdb7 > > src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java > 333db30d7b0b229b8e66a91a0786a5c22268c299 > src/test/java/org/apache/aurora/scheduler/resources/ResourceSlotTest.java > 842572c0b42de1c287f3de22bc8ae9f2fc940fc5 > src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java > e0cca4b89f2cf9ac32d88f4544ff85c68c62b585 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > 81baa7826896f4179be47c53336e4235b35b97b6 > > Diff: https://reviews.apache.org/r/47050/diff/ > > > Testing > ------- > > ./gradlew -Pq build > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Maxim Khutornenko > >
