----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64304/#review192789 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp Line 1492 (original), 1492 (patched) <https://reviews.apache.org/r/64304/#comment271070> Feel free to send a small patch for this since it's an indpendent change, I can get it committed quickly. src/master/allocator/mesos/hierarchical.cpp Line 2351 (original), 2370 (patched) <https://reviews.apache.org/r/64304/#comment271069> Whoops! Can you put this in the original patch? src/tests/hierarchical_allocator_tests.cpp Lines 1270-1271 (patched) <https://reviews.apache.org/r/64304/#comment271046> Reservations should be accounted towards the quota guarantee/limit even if they are currently unallocated. src/tests/hierarchical_allocator_tests.cpp Lines 1272 (patched) <https://reviews.apache.org/r/64304/#comment271049> After reading through the test, this is the dynamic reservation case only? Should we call this test `QuotaLimitWithDynamicReservation` and have another test for static reservations? Or do you want to test both cases in this test? src/tests/hierarchical_allocator_tests.cpp Lines 1310-1311 (patched) <https://reviews.apache.org/r/64304/#comment271052> This note seems to contradict the test? Is it a copy paste mistake? src/tests/hierarchical_allocator_tests.cpp Lines 1321 (patched) <https://reviews.apache.org/r/64304/#comment271053> As a general comment, be sure to start comments with a capital letter and end them with a period, please do a sweep across this patch for other instances. There are also some typos in the comments, so be sure to do a self-review before publishing a patch. src/tests/hierarchical_allocator_tests.cpp Lines 1330-1331 (patched) <https://reviews.apache.org/r/64304/#comment271054> This comment seems like a copy/paste mistake? I'm not sure you need a settle here. src/tests/hierarchical_allocator_tests.cpp Lines 1347-1348 (patched) <https://reviews.apache.org/r/64304/#comment271055> allocated to the role src/tests/hierarchical_allocator_tests.cpp Lines 1355 (patched) <https://reviews.apache.org/r/64304/#comment271056> What is a satisfied reservation? An allocated reservation? src/tests/hierarchical_allocator_tests.cpp Lines 1358 (patched) <https://reviews.apache.org/r/64304/#comment271058> I think you mean allocated resources + unallocated reservations? src/tests/hierarchical_allocator_tests.cpp Lines 1359 (patched) <https://reviews.apache.org/r/64304/#comment271059> the quota limit src/tests/hierarchical_allocator_tests.cpp Lines 1359-1361 (patched) <https://reviews.apache.org/r/64304/#comment271060> These last two sentences were a bit confusing, I think you can do without them if you update the addition above to reflect the lack of double counting? src/tests/hierarchical_allocator_tests.cpp Lines 1362 (patched) <https://reviews.apache.org/r/64304/#comment271061> Looks like this is already stated at the top, can you remove this? src/tests/hierarchical_allocator_tests.cpp Lines 1363 (patched) <https://reviews.apache.org/r/64304/#comment271062> This test is also just dynamic reservations, so the same comment above applies. Maybe we need to parameterize these tests based on whether to use static or dynamic reservation? It's also not clear to me why we need both tests, instead of just a single test. E.g. quota = R add agent 1 with R resources, reserve them, decline forever add agent 2 with R resources, they should not be allocated revive should only get agent 1 resources offered trigger another allocation cycle (to make sure it's enforced across cycles) agent 2 should not be offered to the role src/tests/hierarchical_allocator_tests.cpp Lines 1401 (patched) <https://reviews.apache.org/r/64304/#comment271064> Ditto, this looks like a copy/paste mistake? src/tests/hierarchical_allocator_tests.cpp Lines 4737-4796 (original), 4902-4956 (patched) <https://reviews.apache.org/r/64304/#comment271063> Can you pull this bit out into a separate patch? We can get this committed independently. src/master/allocator/mesos/hierarchical.cpp Lines 1543-1556 (patched) <https://reviews.apache.org/r/64304/#comment271082> I think ultimately, the sorter needs to consider reserved resources as "allocated", do you have any TODO related to this or any tickets? I guess the "unfairly satisfy guarantees" in [MESOS-4527](https://issues.apache.org/jira/browse/MESOS-4527) means that we couldn't close this ticket until we also track the reservations in the sorters, which will affect the code here (i.e. you might not need to track the reservations separately?) Also, we would need a ticket for reservations breaking fairness for the non-quota roles, I don't think there is one yet. src/master/allocator/mesos/hierarchical.cpp Lines 1569-1572 (original), 1588-1594 (patched) <https://reviews.apache.org/r/64304/#comment271083> I'm weary of introducing new terminology here (i.e. "possession" of resources). Maybe `resourcesChargedAgainstQuota`? src/master/allocator/mesos/hierarchical.cpp Lines 1645-1653 (original), 1673-1681 (patched) <https://reviews.apache.org/r/64304/#comment271081> Can you add a note here about [MESOS-7099](https://issues.apache.org/jira/browse/MESOS-7099)? src/master/allocator/mesos/hierarchical.cpp Lines 1797-1798 (patched) <https://reviews.apache.org/r/64304/#comment271080> How about: ``` // TODO(mzhu): Breaking early here means that we may leave reservations // unallocated. See MESOS-8293. ``` - Benjamin Mahler On Dec. 4, 2017, 11:18 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64304/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2017, 11:18 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael > Park. > > > Bugs: MESOS-4527 > https://issues.apache.org/jira/browse/MESOS-4527 > > > Repository: mesos > > > Description > ------- > > Enforced quota limit in the presence of unallocated reservations. > Also modify the allocation process such that the first stage allocation > is solely for quota roles while the second stage is solely for > non-quota roles. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 20b908cc891f9f9be3045cd9f8be83a11d37ab78 > src/tests/hierarchical_allocator_tests.cpp > 0309074bab180be122c9b0074981e6f69c97feee > > > Diff: https://reviews.apache.org/r/64304/diff/3/ > > > Testing > ------- > > Introduced two dedicated tests. > > > Thanks, > > Meng Zhu > >
