You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/02/04 03:02:15 UTC
Re: Review Request 55870: Update the allocator to handle frameworks
with multiple roles.
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > include/mesos/allocator/allocator.hpp, line 91
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615402#file1615402line91>
> >
> > I think that we also need to reflect this in CHANGELOG to clarify that this interface was updated for multi_role support.
Sounds good, I added to the description of [MESOS-6762](https://issues.apache.org/jira/browse/MESOS-6762).
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 283
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615404#file1615404line283>
> >
> > I think that the reason you want to update here is want to keep consistent, right?
Yes
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1137
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1133>
> >
> > Add `role` here in the log message?
Hm.. not sure what you mean, the role will be displayed when printing the resources. Can you clarify?
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1133-1138
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1133>
> >
> > Add `role` here in the log message?
Ditto here.
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1231-1232
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1231>
> >
> > 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`.
Sure, sounds good.
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1455
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1455>
> >
> > s/role/roles
This is referring to the reserved role, so this should remain singular.
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1495
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line1495>
> >
> > 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`.
Yes, sorry! This became stale as I was writing the code.
> On Jan. 28, 2017, 11:45 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 2059-2061
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line2059>
> >
> > Adding `role` in the log message here to clarify which role on this framework is being filtered?
Sounds good!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55870/#review163388
-----------------------------------------------------------
On Jan. 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 Jan. 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
>
>