----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66870/#review206482 -----------------------------------------------------------
Fix it, then Ship it! Overall looks good, but there's a significant existing issue below that I want to make sure we follow up on. src/master/allocator/mesos/hierarchical.cpp Line 287 (original), 288 (patched) <https://reviews.apache.org/r/66870/#comment289466> It jumps out to the reader as a potential bug that the activate case doesn't have a `reviveRole` call to make it symmetric. Shall we just add one? Is this activate call a no-op? src/master/allocator/mesos/hierarchical.cpp Lines 734-758 (original), 747-772 (patched) <https://reviews.apache.org/r/66870/#comment289467> I'm not familiar with this, but it reads as wrong. Why does clearing the agent filters modify the framework's suppressed roles? That breaks the API contract in which frameworks are in control of their suppression state. I don't think we should tweak their suppression state based on events, we should instead expose those events to schedulers and let them decide. Can you follow up with a ticket / fix for this or ask (benno i think?) to follow up? src/master/allocator/mesos/metrics.hpp Lines 104-105 (patched) <https://reviews.apache.org/r/66870/#comment289465> This is quickly going to be stale when we add the update call that kapil is working on, maybe just say that frameworks can update their roles without saying when? src/master/allocator/mesos/metrics.hpp Lines 111 (patched) <https://reviews.apache.org/r/66870/#comment289459> This wasn't obvious to me at first glance, how about: ``` // Suppresion state metric (boolean 0 or 1) for each role. ``` src/master/allocator/mesos/metrics.cpp Lines 220-224 (patched) <https://reviews.apache.org/r/66870/#comment289463> It's fine for now since we don't have to deal with frameworks with large numbers of roles yet, but `getRoles` copies the roles out from `frameworkInfo` into a set, and we only need to iterate through them here so it's a bit wasteful. A TODO? src/master/allocator/mesos/metrics.cpp Lines 213-215 (original), 238-271 (patched) <https://reviews.apache.org/r/66870/#comment289460> These are not known hot paths (although `addRole()` might be in the future with multi-role frameworks with large numbers of roles), but consider removing the redundant lookups in these if you can keep the code readable. src/master/allocator/mesos/metrics.cpp Lines 242 (patched) <https://reviews.apache.org/r/66870/#comment289461> Do you need the `.`? It should convert implicitly to double? If you do need it, can we do `0.0` and `1.0`? That seems a little more readable? src/master/allocator/mesos/metrics.cpp Lines 250 (patched) <https://reviews.apache.org/r/66870/#comment289462> Ditto here. - Benjamin Mahler On July 24, 2018, 3:40 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66870/ > ----------------------------------------------------------- > > (Updated July 24, 2018, 3:40 p.m.) > > > Review request for mesos, Benjamin Mahler, Gastón Kleiman, Gilbert Song, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Added per-framework metrics for suppressed roles. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 707dd6bd0f255a64d759ce87cbf75be57d86b392 > src/master/allocator/mesos/metrics.hpp > 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b > src/master/allocator/mesos/metrics.cpp > 82990b2dc0b827a43a392d898667eaf58c77ea36 > > > Diff: https://reviews.apache.org/r/66870/diff/6/ > > > Testing > ------- > > Testing details can be found at the end of this chain. > > > Thanks, > > Greg Mann > >
