mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 59860: Prevent allocating non-capable agents' resources to hierarchical roles.
Date Thu, 08 Jun 2017 18:24:04 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59860/#review177341
-----------------------------------------------------------



Don't we also need to prevent the creation of volumes under hierarchical roles via the operator
endpoints?


src/master/allocator/mesos/hierarchical.cpp
Lines 2087 (patched)
<https://reviews.apache.org/r/59860/#comment250885>

    Do we really want to issue a warning here during every allocation cycle? This could cause
a ton of log messages. (I realize we use the same log level for the multi-role case, but that
is also debatable, IMO.)



src/tests/upgrade_tests.cpp
Lines 558 (patched)
<https://reviews.apache.org/r/59860/#comment250900>

    Can be one line.



src/tests/upgrade_tests.cpp
Lines 614 (patched)
<https://reviews.apache.org/r/59860/#comment250897>

    Why `MULTI_ROLE`?



src/tests/upgrade_tests.cpp
Lines 631 (patched)
<https://reviews.apache.org/r/59860/#comment250899>

    Settle should be unnecessary.



src/tests/upgrade_tests.cpp
Lines 635 (patched)
<https://reviews.apache.org/r/59860/#comment250898>

    Triggering batch alloc should be unnecessary.



src/tests/upgrade_tests.cpp
Lines 641 (patched)
<https://reviews.apache.org/r/59860/#comment250896>

    Nit: I like `offers->front()` better.



src/tests/upgrade_tests.cpp
Lines 666 (patched)
<https://reviews.apache.org/r/59860/#comment250895>

    Triggering a batch alloc should be unnecessary.



src/tests/upgrade_tests.cpp
Lines 699 (patched)
<https://reviews.apache.org/r/59860/#comment250894>

    Can be one line.



src/tests/upgrade_tests.cpp
Lines 755 (patched)
<https://reviews.apache.org/r/59860/#comment250892>

    Any particular reason we're setting `MULTI_ROLE` capability here?



src/tests/upgrade_tests.cpp
Lines 768 (patched)
<https://reviews.apache.org/r/59860/#comment250893>

    Settle should be unnecessary.



src/tests/upgrade_tests.cpp
Lines 772 (patched)
<https://reviews.apache.org/r/59860/#comment250888>

    This seems racy: when the framework registers, it will be offered resources immediately
(w/o waiting for the next batch allocation). So I'd remove the `Clock::advance(allocation_interval)`
below, and move the `resourceOffers` expectation back to before starting the driver.



src/tests/upgrade_tests.cpp
Lines 798 (patched)
<https://reviews.apache.org/r/59860/#comment250889>

    Waiting for a batch alloc should be unnecessary.



src/tests/upgrade_tests.cpp
Lines 802 (patched)
<https://reviews.apache.org/r/59860/#comment250890>

    "Now that the agent has advertised support for hierarchical roles, the framework should
receive an offer for its resources."


- Neil Conway


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