mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 59860: Prevent allocating non-capable agents' resources to hierarchical roles.
Date Sat, 10 Jun 2017 01:48:12 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message