----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40332/#review107607 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 196) <https://reviews.apache.org/r/40332/#comment166846> s/Methods/Functions/Helpers/ Methods is more of a java-ism :-) src/master/allocator/mesos/hierarchical.cpp (lines 177 - 178) <https://reviews.apache.org/r/40332/#comment166833> This first invariant is something we push on the user of the allocator API (i.e. don't call `addSlave()` until after `recover()` is called). The second is an internal invariant we maintain between `initialize()` and `recover()`. Can we clarify this with a comment, maybe even split them out in separate code blocks? I know this seems picky, but we try not to use `CHECK`s for external invariants (like L177). We happen to do so currently in the allocator because it is so tightly integrated with the master. Ideally we can remove these external ones once we refactor; however, we would want to keep the internal one. Does this make sense? src/master/allocator/mesos/hierarchical.cpp (line 184) <https://reviews.apache.org/r/40332/#comment166834> new line. src/master/allocator/mesos/hierarchical.cpp (line 187) <https://reviews.apache.org/r/40332/#comment166835> s/after/after a/ src/master/allocator/mesos/hierarchical.cpp (lines 191 - 193) <https://reviews.apache.org/r/40332/#comment166838> Is there a ticket for this? I think we'll want to do this for the MVP. Thoughts? src/master/allocator/mesos/hierarchical.cpp (line 198) <https://reviews.apache.org/r/40332/#comment166839> s/Record a/Record the/ src/master/allocator/mesos/hierarchical.cpp (lines 199 - 200) <https://reviews.apache.org/r/40332/#comment166840> Let's drop the `this`, and rename the parameter with a `_`. src/master/allocator/mesos/hierarchical.cpp (line 412) <https://reviews.apache.org/r/40332/#comment166843> Should we `CHECK(!paused || expectedAgentCount.isSome());` above this? src/master/allocator/mesos/hierarchical.cpp (lines 413 - 418) <https://reviews.apache.org/r/40332/#comment166844> Rather than doing the math here (which I believe we're missing corresponding entries for in `removeSlave()`?) why not test whether `slaves.size() >= expectedAgentCount()` ? I think it is more clear, and less error prone. Let's log when this condition is met? src/master/allocator/mesos/hierarchical.cpp (lines 988 - 997) <https://reviews.apache.org/r/40332/#comment166836> Let's most definitely log this :-) It should happen rarely, so it's not a big deal to log. Considering the conversations we had with other developers, this is definitely something they will want to know when debugging ;-) src/master/allocator/mesos/hierarchical.cpp (lines 994 - 997) <https://reviews.apache.org/r/40332/#comment166845> Does it make sense to `CHECK(paused)` here? If not, why not? I think making these semantics as clear as possible is crucial for such a high impact change. src/master/allocator/mesos/hierarchical.cpp (lines 1009 - 1010) <https://reviews.apache.org/r/40332/#comment166837> Let's log that we are skipping allocation due to a paused allocator. Same for the keyed version of this function below. - Joris Van Remoortere On Nov. 23, 2015, 4:11 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40332/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2015, 4:11 p.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > Joseph Wu, and Qian Zhang. > > > Bugs: MESOS-3981 > https://issues.apache.org/jira/browse/MESOS-3981 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > c65fe35198b846da2dc959dd467a21ff6edd30a9 > src/master/allocator/mesos/hierarchical.cpp > 2765526047767cbd19d13c11ecfa6e90c505b3a7 > > Diff: https://reviews.apache.org/r/40332/diff/ > > > Testing > ------- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
