You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/02/27 22:20:36 UTC
Review Request 57111: Updated the allocator to handle frameworks that
change its roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
Diff: https://reviews.apache.org/r/57111/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167928
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 27, 2017, 10:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
>
> Diff: https://reviews.apache.org/r/57111/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/
-----------------------------------------------------------
(Updated March 6, 2017, 3:52 a.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 0bb24be2761bde6d1baad69f5654029f3ceed553
src/master/allocator/mesos/hierarchical.cpp dcafc79421fec179f274aff05da516e64447c924
Diff: https://reviews.apache.org/r/57111/diff/5/
Changes: https://reviews.apache.org/r/57111/diff/4-5/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167949
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp
Lines 518-528 (patched)
<https://reviews.apache.org/r/57111/#comment239946>
Ditto the suggestion on the previous patch for renaming these to avoid confusion.
- Benjamin Mahler
On March 5, 2017, 3:36 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated March 5, 2017, 3:36 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated the allocator to handle frameworks that change its roles.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0bb24be2761bde6d1baad69f5654029f3ceed553
> src/master/allocator/mesos/hierarchical.cpp dcafc79421fec179f274aff05da516e64447c924
>
>
> Diff: https://reviews.apache.org/r/57111/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/
-----------------------------------------------------------
(Updated March 4, 2017, 7:36 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
Updated the allocator to handle frameworks that change its roles.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 0bb24be2761bde6d1baad69f5654029f3ceed553
src/master/allocator/mesos/hierarchical.cpp dcafc79421fec179f274aff05da516e64447c924
Diff: https://reviews.apache.org/r/57111/diff/4/
Changes: https://reviews.apache.org/r/57111/diff/3-4/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/
-----------------------------------------------------------
(Updated March 4, 2017, 1:32 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
Updated the allocator to handle frameworks that change its roles.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 0bb24be2761bde6d1baad69f5654029f3ceed553
src/master/allocator/mesos/hierarchical.cpp dcafc79421fec179f274aff05da516e64447c924
Diff: https://reviews.apache.org/r/57111/diff/3/
Changes: https://reviews.apache.org/r/57111/diff/2-3/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/
-----------------------------------------------------------
(Updated March 4, 2017, 11:47 a.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Addressed bmahler's comments.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description (updated)
-------
Updated the allocator to handle frameworks that change its roles.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.hpp 0bb24be2761bde6d1baad69f5654029f3ceed553
src/master/allocator/mesos/hierarchical.cpp dcafc79421fec179f274aff05da516e64447c924
Diff: https://reviews.apache.org/r/57111/diff/2/
Changes: https://reviews.apache.org/r/57111/diff/1-2/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 27, 2017, 2:30 p.m., Michael Park wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1069 (patched)
> > <https://reviews.apache.org/r/57111/diff/1/?file=1650790#file1650790line1106>
> >
> > This is way more expensive than it needs to be. There's currently not a function to test for an empty allocation. Would it be worth adding it?
Turns out we actually return a const-ref here... so. never mind.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review166955
-----------------------------------------------------------
On Feb. 27, 2017, 2:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
>
> Diff: https://reviews.apache.org/r/57111/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review166955
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 429)
<https://reviews.apache.org/r/57111/#comment239046>
This is because when a partitioned agent comes back with a task, we need to conditionally re-add it back. With just a count, we have no way to tell whether the framework for which the task came back for, is already in the count or not.
src/master/allocator/mesos/hierarchical.hpp (lines 513 - 523)
<https://reviews.apache.org/r/57111/#comment239048>
These guys tie the notion of what it means for a framework to exist in a role within the hierarchical allocator. That is, `activeRoles`, `roleSorter`, `frameworkSorters`, `frameworkSorters.at(role)` (if one exists), and `metrics`.
If this is okay, I'll add the above blurb as a comment.
src/master/allocator/mesos/hierarchical.cpp (line 1069)
<https://reviews.apache.org/r/57111/#comment239047>
This is way more expensive than it needs to be. There's currently not a function to test for an empty allocation. Would it be worth adding it?
- Michael Park
On Feb. 27, 2017, 2:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
> Diff: https://reviews.apache.org/r/57111/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 3, 2017, 5:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 458 (original), 463 (patched)
> > <https://reviews.apache.org/r/57111/diff/1/?file=1650790#file1650790line500>
> >
> > Can you commit this separately?
Committed separately here: https://github.com/apache/mesos/commit/a152828e13fcebb49bd813d391d3836144bd817b
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167882
-----------------------------------------------------------
On Feb. 27, 2017, 2:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
>
> Diff: https://reviews.apache.org/r/57111/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 3, 2017, 5:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 429-432 (original), 394-400 (patched)
> > <https://reviews.apache.org/r/57111/diff/1/?file=1650790#file1650790line431>
> >
> > Ditto from last review, could we simplify with a - operator in stout/set.hpp?
> >
> > ```
> > const set<string> removedRoles = oldRoles - newRoles;
> > ```
Replied in the previous patch.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167882
-----------------------------------------------------------
On Feb. 27, 2017, 2:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
>
> Diff: https://reviews.apache.org/r/57111/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57111: Updated the allocator to handle frameworks
that change its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167882
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp
Lines 426-429 (original), 426-429 (patched)
<https://reviews.apache.org/r/57111/#comment239845>
Ditto suggestion from previous review for updating this comment, and also removing the notion of "active" here.
src/master/allocator/mesos/hierarchical.cpp
Lines 429-432 (original), 394-400 (patched)
<https://reviews.apache.org/r/57111/#comment239864>
Ditto from last review, could we simplify with a - operator in stout/set.hpp?
```
const set<string> removedRoles = oldRoles - newRoles;
```
src/master/allocator/mesos/hierarchical.cpp
Lines 417-423 (patched)
<https://reviews.apache.org/r/57111/#comment239865>
Ditto from last review, could we simplify with a - operator in stout/set.hpp?
```
const set<string> addedRoles = newRoles - oldRoles;
```
src/master/allocator/mesos/hierarchical.cpp
Lines 426-429 (patched)
<https://reviews.apache.org/r/57111/#comment239883>
Hm.. this was a bit tough to grasp for me, how about something like:
```
// Add the framework to the role. It's possible that we're already
// tracking this role for the framework because a framework can
// unsubscribe from a role while it still has resources allocated
// to the role.
```
src/master/allocator/mesos/hierarchical.cpp
Line 458 (original), 463 (patched)
<https://reviews.apache.org/r/57111/#comment239886>
Can you commit this separately?
src/master/allocator/mesos/hierarchical.cpp
Lines 470-473 (patched)
<https://reviews.apache.org/r/57111/#comment239895>
Maybe something a bit more general, since this isn't the only case where this is possible and that might confuse the reader. It's also possible when there is no partition involved, when the master is re-registering an agent.
E.g.
```
// The framework has resources allocated to this role but it may
// or may not be subscribed to the role. Either way, we need to
// track the framework under the role.
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1065-1071 (patched)
<https://reviews.apache.org/r/57111/#comment239912>
Hm.. maybe we can use "start tracking" and "stop tracking" in these comments since "adding" "removing" from the role seems a little less clear to me. E.g.
```
Stop tracking the framework under this role if it's no longer subscribed and no longer has resources allocated to the role.
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1066 (patched)
<https://reviews.apache.org/r/57111/#comment239896>
s/no/no longer any/
src/master/allocator/mesos/hierarchical.cpp
Lines 2186-2187 (patched)
<https://reviews.apache.org/r/57111/#comment239855>
Subscribe *or have resources allocated to this role*, right?
src/master/allocator/mesos/hierarchical.cpp
Lines 2191 (patched)
<https://reviews.apache.org/r/57111/#comment239861>
Do you want a CHECK in front of this insert, since you seem to be check guarding every other insert in this function?
src/master/allocator/mesos/hierarchical.cpp
Lines 2218 (patched)
<https://reviews.apache.org/r/57111/#comment239862>
This condition isn't quite right anymore, maybe something like:
```
If no more frameworks are subscribed to this role or have resources allocated to this role, ...
```
src/master/allocator/mesos/hierarchical.cpp
Lines 2228 (patched)
<https://reviews.apache.org/r/57111/#comment239849>
Did you want to use an `.at()` here?
- Benjamin Mahler
On Feb. 27, 2017, 10:20 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 10:20 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65
> src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c
>
>
> Diff: https://reviews.apache.org/r/57111/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>