mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.
Date Sat, 28 Jan 2017 11:45:31 GMT

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




include/mesos/allocator/allocator.hpp (line 91)
<https://reviews.apache.org/r/55870/#comment234820>

    I think that we also need to reflect this in CHANGELOG to clarify that this interface
was updated for multi_role support.



src/master/allocator/mesos/hierarchical.hpp (line 232)
<https://reviews.apache.org/r/55870/#comment234821>

    How about update the comments a bit?
    ```
    // Remove an offer filter for the specified role of a framework.
    ```



src/master/allocator/mesos/hierarchical.hpp (lines 251 - 252)
<https://reviews.apache.org/r/55870/#comment234822>

    How about update the comments a bit?
    ```
    // Returns true if there is a resource offer filter for the
    // speficied role of the framework on this slave.
    ```



src/master/allocator/mesos/hierarchical.hpp (line 283)
<https://reviews.apache.org/r/55870/#comment234823>

    I think that the reason you want to update here is want to keep consistent, right?



src/master/allocator/mesos/hierarchical.cpp (line 428)
<https://reviews.apache.org/r/55870/#comment234824>

    s/a new 'role'/new 'roles'



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1128)
<https://reviews.apache.org/r/55870/#comment234841>

    Add `role` here in the log message?



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1129)
<https://reviews.apache.org/r/55870/#comment234842>

    Add `role` here in the log message?



src/master/allocator/mesos/hierarchical.cpp (lines 1167 - 1168)
<https://reviews.apache.org/r/55870/#comment234843>

    A question here: Why not call `resources.unallocate()` at #1130? 
    
    This can make the logic of `allocate` and `recoverResources` as pair: `allocate` will
set allocation info while `recoverResources` will clear allocatio info.



src/master/allocator/mesos/hierarchical.cpp (lines 1219 - 1220)
<https://reviews.apache.org/r/55870/#comment234836>

    Nit, how about update the message a bit:
    
    ```
    Suppressed offers for roles {r1,r2} in framework xx-xx-xx-xx
    ```
    
    Ditto for the log message in `reviveOffers`.



src/master/allocator/mesos/hierarchical.cpp (line 1440)
<https://reviews.apache.org/r/55870/#comment234837>

    s/role/roles



src/master/allocator/mesos/hierarchical.cpp (line 1480)
<https://reviews.apache.org/r/55870/#comment234838>

    Why highlight the `unallocate()` here, I think that this was already done via `Resources::createStrippedScalarQuantity`
and the resources being `flatten` here should not have `allocation info`.



src/master/allocator/mesos/hierarchical.cpp (lines 2044 - 2046)
<https://reviews.apache.org/r/55870/#comment234839>

    Adding `role` in the log message here to clarify which role on this framework is being
filtered?



src/master/allocator/mesos/hierarchical.cpp (lines 2044 - 2046)
<https://reviews.apache.org/r/55870/#comment234840>

    Add `role` in the log message here to clarify which role on this framework is being filtered?


- Guangya Liu


On 一月 25, 2017, 9:55 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> -----------------------------------------------------------
> 
> (Updated 一月 25, 2017, 9:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6635
>     https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d

>   src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a

>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> -------
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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