----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66044/#review200846 -----------------------------------------------------------
Fix it, then Ship it! Looks good! Just thinking we probably should pull the shrink function up and unit test it? Feel free to do that in a second patch. src/master/allocator/mesos/hierarchical.cpp Lines 1600 (patched) <https://reviews.apache.org/r/66044/#comment281749> To enable move performance improvements later, we can do: ``` result += std::move(resource); ``` src/master/allocator/mesos/hierarchical.cpp Lines 1607-1610 (patched) <https://reviews.apache.org/r/66044/#comment281751> I assume this continues to work as the value scalar goes negative? We should probably pull this up into the header and unit test it? src/master/allocator/mesos/hierarchical.cpp Lines 1608-1609 (patched) <https://reviews.apache.org/r/66044/#comment281750> Ditto here for moves: ``` targetScalarQuantites[resource.name()] -= limitScalar.get(); result += std::move(resource); ``` src/master/allocator/mesos/hierarchical.cpp Line 1868 (original), 1883 (patched) <https://reviews.apache.org/r/66044/#comment281753> s/(/ (/ src/master/allocator/mesos/hierarchical.cpp Line 1870 (original), 1885 (patched) <https://reviews.apache.org/r/66044/#comment281754> Can you use CHECK_NOTNONE here instead of .get()? src/master/allocator/mesos/hierarchical.cpp Lines 1875-1876 (original), 1888-1889 (patched) <https://reviews.apache.org/r/66044/#comment281758> I think it's clear from the code without this comment, the comment actually threw me off because I was trying to figure out what "this time" was referring to (e.g. stage 1?). src/master/allocator/mesos/hierarchical.cpp Lines 1927 (patched) <https://reviews.apache.org/r/66044/#comment281760> Ditto here s/(/ (/ src/master/allocator/mesos/hierarchical.cpp Lines 1929 (patched) <https://reviews.apache.org/r/66044/#comment281761> Ditto here CHECK_NOTNONE - Benjamin Mahler On March 19, 2018, 11:48 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66044/ > ----------------------------------------------------------- > > (Updated March 19, 2018, 11:48 p.m.) > > > Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael > Park, and Till Toenshoff. > > > Repository: mesos > > > Description > ------- > > Introduced a `shrinkResources()` lambda in allocator > so that the same resource chopping logic can be re-used > in the future, in particular, when introducing the quota > limit. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 32e88952101d8dbbae9728478b1f5663bf46c3bb > > > Diff: https://reviews.apache.org/r/66044/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
