----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59860/#review177534 -----------------------------------------------------------
Fix it, then Ship it! Modulo existing comments. src/master/allocator/mesos/hierarchical.cpp Lines 2086 (patched) <https://reviews.apache.org/r/59860/#comment251166> This is in a hot path in the allocation loop. You might want to flip this conditional to avoid searching every role when the agents are upgraded to hierarchical role: ``` if (!slave.capabilities.hierarchicalRole && strings::contains(role, "/")) { ``` src/master/allocator/mesos/hierarchical.cpp Lines 2089 (patched) <https://reviews.apache.org/r/59860/#comment251165> is not HIERARCHICAL_ROLE capable? src/tests/upgrade_tests.cpp Lines 537-542 (patched) <https://reviews.apache.org/r/59860/#comment251168> Thanks for describing how the tests work! Very helpful to the reader src/tests/upgrade_tests.cpp Lines 562-576 (patched) <https://reviews.apache.org/r/59860/#comment251169> This needs to come before you start the slave, no? src/tests/upgrade_tests.cpp Lines 578-579 (patched) <https://reviews.apache.org/r/59860/#comment251170> I wonder if we could have 0 initial delay by default for the tests so that you don't have to deal with this here. Want to file a ticket? :D src/tests/upgrade_tests.cpp Lines 610-629 (patched) <https://reviews.apache.org/r/59860/#comment251171> In order to simplify this test, we could file another ticket for having a set of testing schedulers that have common behavior: no-op, task launching, etc. So that we don't have to be as verbose setting up expectations around registration and offers. We could have this ticket and the previous one I suggested be underneath an epic to simplify our tests. I can also file them if you like, let me know! src/tests/upgrade_tests.cpp Lines 703-717 (patched) <https://reviews.apache.org/r/59860/#comment251172> It would be nice to put these above the starting of the slave, see my previous comment about being able to avoid the need to advance the clock. - Benjamin Mahler On June 7, 2017, 1:54 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59860/ > ----------------------------------------------------------- > > (Updated June 7, 2017, 1:54 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7633 > https://issues.apache.org/jira/browse/MESOS-7633 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 > src/tests/upgrade_tests.cpp b07426fa1e402c88a8a647eafdb77f6ebadd9959 > > > Diff: https://reviews.apache.org/r/59860/diff/1/ > > > Testing > ------- > > New tests + `make check`. > > > Thanks, > > Michael Park > >
