----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70618/#review215354 -----------------------------------------------------------
Just a partial review, will post the rest later src/master/allocator/mesos/hierarchical.hpp Line 110 (original), 111 (patched) <https://reviews.apache.org/r/70618/#comment302016> space before brace Did I mention this in the last patch? Did you rebase? src/master/allocator/mesos/hierarchical.hpp Lines 112 (patched) <https://reviews.apache.org/r/70618/#comment302017> You have a user-defined constructor declared below. So this is not strictly needed. And (correct me if I am wrong) I do not think our style guide requires this, I would suggest removing this for brevity. src/master/allocator/mesos/hierarchical.hpp Lines 115 (patched) <https://reviews.apache.org/r/70618/#comment302018> Put this in the line above. src/master/allocator/mesos/hierarchical.hpp Lines 118-120 (original), 128-133 (patched) <https://reviews.apache.org/r/70618/#comment302019> The original comment is verbose and outdated. Let's take this opportunity to improve it. Also, let's make an effort to align the comment length :) - Let's remove the ties to DRF (as it can be any sorter algorithm) - Let's remove the details of level and stage, readers who are familiar with the matter do not need to comment, those who are not will just be confused. Also, the structure of the code could change in the future. - The comment regarding resource pool is no longer true due to: https://github.com/apache/mesos/commit/a3626219b402ff837192208b6d35946b3a069ce6 How about: // The sorter determines the resource allocation order of the // frameworks subscribed under this role. src/master/allocator/mesos/hierarchical.cpp Lines 358-359 (original), 358-359 (patched) <https://reviews.apache.org/r/70618/#comment302020> // FrameworkID may not be present in RoleInfo because the // framework was previously deactivated and never re-added. src/master/allocator/mesos/hierarchical.cpp Line 367 (original), 367 (patched) <https://reviews.apache.org/r/70618/#comment302021> Not yours, but this could be just a reference? const hashmap<SlaveID, Resources>& allocation = ... Want to add a quick patch for that? src/master/allocator/mesos/hierarchical.cpp Line 888 (original), 888 (patched) <https://reviews.apache.org/r/70618/#comment302022> hmm, it is unfortunate that we need to make a copy here due to the sorter interface.... src/master/allocator/mesos/hierarchical.cpp Lines 982-1000 (original), 981-999 (patched) <https://reviews.apache.org/r/70618/#comment302023> We should just use `ResourceQuantities` for these. Feel free to do this in another patch. src/master/allocator/mesos/hierarchical.cpp Line 1369 (original), 1367 (patched) <https://reviews.apache.org/r/70618/#comment302024> suppressingRoles src/master/allocator/mesos/hierarchical.cpp Line 1373 (original), 1372 (patched) <https://reviews.apache.org/r/70618/#comment302025> newline not necessary? src/master/allocator/mesos/hierarchical.cpp Line 1395 (original), 1393 (patched) <https://reviews.apache.org/r/70618/#comment302026> revivingRoles src/master/allocator/mesos/hierarchical.cpp Line 1402 (original), 1401 (patched) <https://reviews.apache.org/r/70618/#comment302027> newline not necessary? src/master/allocator/mesos/hierarchical.cpp Lines 1708-1709 (original), 1706-1709 (patched) <https://reviews.apache.org/r/70618/#comment302028> The logic here is not related to this patch. I am OK with either doing this in the previous patch or keep it as is. src/master/allocator/mesos/hierarchical.cpp Line 1714 (original), 1714 (patched) <https://reviews.apache.org/r/70618/#comment302030> This should be in the previous patch (I just raised an issue in the patch) src/master/allocator/mesos/hierarchical.cpp Line 1853 (original), 1853 (patched) <https://reviews.apache.org/r/70618/#comment302032> Not yours, but this comment is tied to DRF, we should remove/modify it. src/master/allocator/mesos/hierarchical.cpp Lines 2169 (patched) <https://reviews.apache.org/r/70618/#comment302034> space before braces src/master/allocator/mesos/hierarchical.cpp Lines 2171 (patched) <https://reviews.apache.org/r/70618/#comment302033> newline here - Meng Zhu On May 16, 2019, 7:17 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70618/ > ----------------------------------------------------------- > > (Updated May 16, 2019, 7:17 a.m.) > > > Review request for mesos, Benjamin Mahler and Meng Zhu. > > > Bugs: MESOS-9701 > https://issues.apache.org/jira/browse/MESOS-9701 > > > Repository: mesos > > > Description > ------- > > This patch replaces a hashmap<Owned<Sorter>> used for tracking > framework sorters with a unique_ptr<Sorter> inside of the RoleInfo. > This simplifies the framework tracking logic and slightly improves > performance. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > c2058baca5159da4cdcab77afd5de3c0d5ae6c48 > src/master/allocator/mesos/hierarchical.cpp > 875cfcd091f5f58afb89e752da5100a75828dd67 > > > Diff: https://reviews.apache.org/r/70618/diff/3/ > > > Testing > ------- > > make check > > Benchmarking: 5 runs of > BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5 with the > optimized build. > > BEFORE (master): > > Added 3000 agents in 57.444301ms > Added 3000 frameworks in 15.100499419secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 13.359926851secs > Made 0 allocation in 12.147765014secs > > Added 3000 agents in 58.651887ms > Added 3000 frameworks in 14.485925735secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 13.6526783secs > Made 0 allocation in 12.138439924secs > > Added 3000 agents in 59.028581ms > Added 3000 frameworks in 14.72945866secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.739135078secs > Made 0 allocation in 12.673302496secs > > Added 3000 agents in 59.050577ms > Added 3000 frameworks in 14.78273576secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 13.20400492secs > Made 0 allocation in 13.289808943secs > > Added 3000 agents in 58.629888ms > Added 3000 frameworks in 14.714786337secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 13.721094744secs > Made 0 allocation in 12.688377237secs > > ----------------------------------- > AFTER (master + previous patch https://reviews.apache.org/r/70591/ + this > patch): > > Added 3000 agents in 58.04155ms > Added 3000 frameworks in 13.694651977secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.417541213secs > Made 0 allocation in 12.130755905secs > > Added 3000 agents in 55.246813ms > Added 3000 frameworks in 13.460479936secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.393765514secs > Made 0 allocation in 12.196981426secs > > Added 3000 agents in 58.013477ms > Added 3000 frameworks in 13.69361015secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.245916159secs > Made 0 allocation in 11.699553888secs > > Added 3000 agents in 57.163681ms > Added 3000 frameworks in 13.738218369secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.809206431secs > Made 0 allocation in 12.400140164secs > > Added 3000 agents in 57.942087ms > Added 3000 frameworks in 13.836390727secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter > Made 3856 allocations in 12.75595625secs > Made 0 allocation in 11.939263998secs > > > Thanks, > > Andrei Sekretenko > >
