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