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:19:52 UTC
Review Request 57110: Updated the master to handle frameworks that
changes its roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
Diff: https://reviews.apache.org/r/57110/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 27, 2017, 2:25 p.m., Michael Park wrote:
> > src/master/master.cpp
> > Lines 5992-5993 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line6004>
> >
> > Resolve this before committing.
Addressed by https://reviews.apache.org/r/57282/
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review166954
-----------------------------------------------------------
On Feb. 27, 2017, 2:19 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:19 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
>
> Diff: https://reviews.apache.org/r/57110/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 27, 2017, 10:25 p.m., Michael Park wrote:
> > src/master/master.hpp
> > Lines 2452-2461 (original), 2515-2524 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650787#file1650787line2536>
> >
> > @bmahler: This allows even non-`MULTI_ROLE` frameworks to change their roles. Is this meant to be supported?
Yeah I think we should support it, since it falls out naturally and is useful (we've been asked for this several times and it's hard for a framework to change its role).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review166954
-----------------------------------------------------------
On Feb. 27, 2017, 10:19 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 10:19 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
>
> Diff: https://reviews.apache.org/r/57110/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review166954
-----------------------------------------------------------
src/master/master.hpp (lines 2515 - 2524)
<https://reviews.apache.org/r/57110/#comment239044>
@bmahler: This allows even non-`MULTI_ROLE` frameworks to change their roles. Is this meant to be supported?
src/master/master.cpp (lines 5992 - 5993)
<https://reviews.apache.org/r/57110/#comment239045>
Resolve this before committing.
- Michael Park
On Feb. 27, 2017, 2:19 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 2:19 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
> Diff: https://reviews.apache.org/r/57110/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 6, 2017, 4:07 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 master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/master.hpp 47c5e61606fa7d971b763c97f2950799ead00e5f
src/master/master.cpp 3fa5438c74c852649179ed5a93202d6c050bd52a
Diff: https://reviews.apache.org/r/57110/diff/10/
Changes: https://reviews.apache.org/r/57110/diff/9-10/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review168031
-----------------------------------------------------------
Fix it, then Ship it!
New function names look good.
src/master/master.hpp
Lines 2246 (patched)
<https://reviews.apache.org/r/57110/#comment240039>
Can we take a const ref here to avoid the copy?
src/master/master.hpp
Lines 2413-2414 (patched)
<https://reviews.apache.org/r/57110/#comment240040>
Ditto here.
- Benjamin Mahler
On March 6, 2017, 11:37 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 11:37 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 master to handle frameworks that changes its roles.
>
>
> Diffs
> -----
>
> src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
>
>
> Diff: https://reviews.apache.org/r/57110/diff/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 6, 2017, 3:37 a.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description (updated)
-------
Updated the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
Diff: https://reviews.apache.org/r/57110/diff/9/
Changes: https://reviews.apache.org/r/57110/diff/8-9/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 6, 2017, 2:57 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/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
Diff: https://reviews.apache.org/r/57110/diff/8/
Changes: https://reviews.apache.org/r/57110/diff/7-8/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 401-412 (patched)
> > <https://reviews.apache.org/r/57110/diff/7/?file=1656423#file1656423line401>
> >
> > Since this is effectively `stopTrackingAllocationToRole`, should we be CHECKing the invariants? i.e. that there is no more allocation and that we're not subscribed to the role
I've added `CHECK(roles.count(role) > 0);` to `trackUnderRole`, and
added the following to `untrackUnderRole`:
```cpp
// NOTE: Ideally we would also `CHECK` that we're not currently subscribed
// to the role. We don't do this currently because this function is used in
// `Master::removeFramework` where we're still subscribed to `roles`.
auto allocatedToRole = [&role](const Resource& resource) {
return resource.allocation_info().role() == role;
};
CHECK(totalUsedResources.filter(allocatedToRole).empty());
CHECK(totalOfferedResources.filter(allocatedToRole).empty());
```
> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/quota_handler.cpp
> > Line 135 (original), 135 (patched)
> > <https://reviews.apache.org/r/57110/diff/7/?file=1656424#file1656424line135>
> >
> > Not yours, but do you want commit a change to use .at here?
https://reviews.apache.org/r/57331/
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167948
-----------------------------------------------------------
On March 6, 2017, 2:57 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 2:57 a.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/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
>
>
> Diff: https://reviews.apache.org/r/57110/diff/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 1785-1786 (original), 1789-1793 (patched)
> > <https://reviews.apache.org/r/57110/diff/7/?file=1656422#file1656422line1789>
> >
> > It looks like we should have pulled out this rename to into a separate patch to have this be a bit cleaner, oh well.
https://reviews.apache.org/r/57329/
> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Line 2423 (original), 2467 (patched)
> > <https://reviews.apache.org/r/57110/diff/7/?file=1656422#file1656422line2490>
> >
> > Do you want to pull this 'source' to 'newInfo' rename patch out to unclutter this diff?
https://reviews.apache.org/r/57330/
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167948
-----------------------------------------------------------
On March 6, 2017, 2:57 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 2:57 a.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/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
>
>
> Diff: https://reviews.apache.org/r/57110/diff/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 2652-2654 (patched)
> > <https://reviews.apache.org/r/57110/diff/7/?file=1656422#file1656422line2702>
> >
> > Per the other suggestions, perhaps something like:
> >
> > ```
> > bool trackingAllocationToRole(const std::string& role) const;
> > void trackAllocationToRole(const std::string& role);
> > void stopTrackingAllocationToRole(const std::string& role);
> > ```
I definitely agree that we need to change this. What do you think of:
```cpp
bool isTrackedUnderRole(const std::string& role);
void trackUnderRole(const std::string& role);
void untrackUnderRole(const std::string& role);
```
Since we're keeping track of the framework under a role. That is,
a framework subscribed to a role without any allocation is still
tracked under the role.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167948
-----------------------------------------------------------
On March 4, 2017, 7:55 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 4, 2017, 7:55 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 master to handle frameworks that changes its roles.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
> src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
> src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
> src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
>
>
> Diff: https://reviews.apache.org/r/57110/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167948
-----------------------------------------------------------
Couple of high level suggestions:
(1) Looks like some patches could be pulled out of this to unclutter the diff? (e.g. the 'activeRoles' and 'source' renames)
(2) The use of "subscribe" in the helpers seems confusing, see below.
src/master/master.hpp
Lines 1785-1786 (original), 1789-1793 (patched)
<https://reviews.apache.org/r/57110/#comment239932>
It looks like we should have pulled out this rename to into a separate patch to have this be a bit cleaner, oh well.
src/master/master.hpp
Lines 2248-2250 (patched)
<https://reviews.apache.org/r/57110/#comment239934>
Hm.. well this sounds misleading, since it seems to read as we're subscribing the framework to the role which means offers will be sent, but that's not the case? Shall we call this something different? Like `trackRole`?
src/master/master.hpp
Lines 2282-2286 (patched)
<https://reviews.apache.org/r/57110/#comment239937>
This seems like another spot of confusion with the use of "subscribed" in the helpers introduced in this patch. Here I would expect `roles.count(role) == 0` to be equivalent to `!isSubscribed(role)`.
Also, calling `unsubscribe(role)` here sounds equivalent to the framework having removed its role, but it's just stopping to track allocation for the role?
src/master/master.hpp
Lines 2408-2419 (patched)
<https://reviews.apache.org/r/57110/#comment239938>
I guess given the phrasing here, using the word "track" might be less confusing for these helpers.. e.g.
```
if (!executorInfo.resources().empty()) {
std::string role =
executorInfo.resources().begin()->allocation_info().role();
if (!trackingAllocation(role)) {
trackAllocation(role);
}
}
```
src/master/master.hpp
Lines 2438-2440 (patched)
<https://reviews.apache.org/r/57110/#comment239939>
We probably want to use consistent language here, "remove ourself from the role" implies the other locations should say "add ourself to the role" but since adding and removing seems a bit vague, this case might be clearer as "stop tracking the allocation for the role".
src/master/master.hpp
Line 2423 (original), 2467 (patched)
<https://reviews.apache.org/r/57110/#comment239943>
Do you want to pull this 'source' to 'newInfo' rename patch out to unclutter this diff?
src/master/master.hpp
Lines 2652-2654 (patched)
<https://reviews.apache.org/r/57110/#comment239940>
Per the other suggestions, perhaps something like:
```
bool trackingAllocationToRole(const std::string& role) const;
void trackAllocationToRole(const std::string& role);
void stopTrackingAllocationToRole(const std::string& role);
```
src/master/master.hpp
Lines 2760 (patched)
<https://reviews.apache.org/r/57110/#comment239942>
Brace on the next line
src/master/master.cpp
Lines 381-412 (patched)
<https://reviews.apache.org/r/57110/#comment239941>
Braces on the next line
src/master/master.cpp
Lines 401-412 (patched)
<https://reviews.apache.org/r/57110/#comment239944>
Since this is effectively `stopTrackingAllocationToRole`, should we be CHECKing the invariants? i.e. that there is no more allocation and that we're not subscribed to the role
src/master/quota_handler.cpp
Line 135 (original), 135 (patched)
<https://reviews.apache.org/r/57110/#comment239933>
Not yours, but do you want commit a change to use .at here?
- Benjamin Mahler
On March 5, 2017, 3:55 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 5, 2017, 3:55 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 master to handle frameworks that changes its roles.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
> src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
> src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
> src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
>
>
> Diff: https://reviews.apache.org/r/57110/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 4, 2017, 7:55 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 master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
Diff: https://reviews.apache.org/r/57110/diff/7/
Changes: https://reviews.apache.org/r/57110/diff/6-7/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 4, 2017, 7:31 p.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 the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
Diff: https://reviews.apache.org/r/57110/diff/6/
Changes: https://reviews.apache.org/r/57110/diff/5-6/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 4, 2017, 1:09 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Updated `Master::updateFramework` and `Framework::updateFrameworkInfo` to return `void`.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
Updated the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
Diff: https://reviews.apache.org/r/57110/diff/5/
Changes: https://reviews.apache.org/r/57110/diff/4-5/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 4, 2017, 11:53 a.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Updated comments to use "start tracking" vs "stop tracking"
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
Updated the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
Diff: https://reviews.apache.org/r/57110/diff/4/
Changes: https://reviews.apache.org/r/57110/diff/3-4/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 3, 2017, 6:08 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Removed the notion of an "active" role.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description
-------
Updated the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp 43e6fadb69cf3989f1c5b36808a2424800e0a0a6
src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a
src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f
Diff: https://reviews.apache.org/r/57110/diff/3/
Changes: https://reviews.apache.org/r/57110/diff/2-3/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167922
-----------------------------------------------------------
src/master/master.hpp
Lines 1785-1786 (original), 1793-1797 (patched)
<https://reviews.apache.org/r/57110/#comment239911>
On second thought, and after discussing offline, let's remove the notion of "active" from the name and the comment here, since "active" has meant:
frameworks: receiving resources
agents: sending resources
and ideally for roles it can consistently mean:
role: receiving resources (i.e. at least one framework wants resources for the role)
Ditto in the allocator patch.
- Benjamin Mahler
On March 3, 2017, 11:10 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 11:10 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 master to handle frameworks that changes its roles.
>
>
> Diffs
> -----
>
> src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp 43e6fadb69cf3989f1c5b36808a2424800e0a0a6
>
>
> Diff: https://reviews.apache.org/r/57110/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 3, 2017, 3:10 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Addressed some of bmahler's comments.
Bugs: MESOS-6627
https://issues.apache.org/jira/browse/MESOS-6627
Repository: mesos
Description (updated)
-------
Updated the master to handle frameworks that changes its roles.
Diffs (updated)
-----
src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
src/master/master.cpp 43e6fadb69cf3989f1c5b36808a2424800e0a0a6
Diff: https://reviews.apache.org/r/57110/diff/2/
Changes: https://reviews.apache.org/r/57110/diff/1-2/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167880
-----------------------------------------------------------
src/master/master.hpp
Lines 1785-1786 (original), 1793-1794 (patched)
<https://reviews.apache.org/r/57110/#comment239842>
We should also update the comment here to reflect the change we're making in this patch, and also to define "active" in this context, e.g.:
```
// We track information about roles that are active
// in the system. A role is active when a framework
// subscribes to the role, and/or when there are resources
// allocated to the role (e.g. some tasks and/or executors
// are consuming resources under the role).
hashmap<std::string, Role*> activeRoles;
```
- Benjamin Mahler
On March 3, 2017, 9:47 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 9:47 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
>
> Diff: https://reviews.apache.org/r/57110/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/
-----------------------------------------------------------
(Updated March 3, 2017, 1:47 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
Diff: https://reviews.apache.org/r/57110/diff/1/
Testing
-------
Thanks,
Michael Park
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 2526 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650787#file1650787line2553>
> >
> > At this point, you have based it on `info` instead of `source`, right? Maybe that's a little clearer since we just adjusted `info`?
Not quite. This is subtle because `getRoles` looks at `capabilities`, and `info.capabilities` is updated later on below.
I thought it's probably cleaner to just initalize off of `source` rather than setting `capabilities` earlier and
have it become order-sensitive.
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3335-3337 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line3347>
> >
> > Should we use .at() for these three lines that intend const access into the map?
Not const-access, but yeah, I agree. We should.
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 5992-5993 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line6004>
> >
> > Per our offline discussion, looks like this belongs in one of the previous reviews that addresses the addition of the FrameworkInfo into the message? Seems like we need to send it always now that it includes the info.
Followed up here: https://reviews.apache.org/r/57282/
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 6085-6091 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650788#file1650788line6097>
> >
> > This could be simplified to:
> >
> > ```
> > const set<string> removedRoles = oldRoles - newRoles;
> > ```
> >
> > If we were to add the `-` operator to the existing operators in stout/set.hpp.
Yeah, but the operators there are defined in the global namespace, and doesn't work for `operator-` because of collisions.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167777
-----------------------------------------------------------
On March 3, 2017, 1:47 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 1:47 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
>
> Diff: https://reviews.apache.org/r/57110/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Michael Park <mp...@apache.org>.
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 2263 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650787#file1650787line2263>
> >
> > This last sentence seems a bit redundant?
Removed with your suggestion above.
> On March 2, 2017, 8:26 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 2320-2324 (original), 2355-2375 (patched)
> > <https://reviews.apache.org/r/57110/diff/1/?file=1650787#file1650787line2355>
> >
> > Can we just call `recoverResources(task)` here?
Yep. Done!
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167777
-----------------------------------------------------------
On March 3, 2017, 3:10 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 3:10 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 master to handle frameworks that changes its roles.
>
>
> Diffs
> -----
>
> src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515
> src/master/master.cpp 43e6fadb69cf3989f1c5b36808a2424800e0a0a6
>
>
> Diff: https://reviews.apache.org/r/57110/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 57110: Updated the master to handle frameworks that
changes its roles.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57110/#review167777
-----------------------------------------------------------
src/master/master.hpp
Lines 2260-2263 (patched)
<https://reviews.apache.org/r/57110/#comment239753>
This case is just one possibility, it's also possible that the framework removes the role with tasks running still, or an agent that still needed to register but wasn't partitioned reports back. So maybe something more general?
```
// It's possible that we're not tracking the task's role for
// this framework if the role is absent from the framework's
// set of roles. In this case, we track the role's allocation
// for this framework.
```
src/master/master.hpp
Lines 2263 (patched)
<https://reviews.apache.org/r/57110/#comment239752>
This last sentence seems a bit redundant?
src/master/master.hpp
Lines 2320-2324 (original), 2355-2375 (patched)
<https://reviews.apache.org/r/57110/#comment239755>
Can we just call `recoverResources(task)` here?
src/master/master.hpp
Lines 2448-2451 (patched)
<https://reviews.apache.org/r/57110/#comment239754>
Ditto here.
src/master/master.hpp
Lines 2526 (patched)
<https://reviews.apache.org/r/57110/#comment239756>
At this point, you have based it on `info` instead of `source`, right? Maybe that's a little clearer since we just adjusted `info`?
src/master/master.cpp
Lines 2637-2638 (original), 2637-2638 (patched)
<https://reviews.apache.org/r/57110/#comment239757>
Do you need the quotes here? This ends up coming out like this:
```
Could not update FrameworkInfo of framework '<ID> (hostname) at 10.10.10.10:5050': ...
```
Whereas I think your intention was to quote the ID. Seems like this is the responsibility of the output operator for `Framework&`.
src/master/master.cpp
Lines 2951-2952 (original), 2941-2942 (patched)
<https://reviews.apache.org/r/57110/#comment239758>
Oh I see it was copied from here.. we should fix the quoting here as well.
src/master/master.cpp
Lines 3301 (patched)
<https://reviews.apache.org/r/57110/#comment239742>
s/std:://
Also I think it fits on one line at that point, like the others below.
src/master/master.cpp
Lines 3335-3337 (patched)
<https://reviews.apache.org/r/57110/#comment239741>
Should we use .at() for these three lines that intend const access into the map?
src/master/master.cpp
Lines 5992-5993 (patched)
<https://reviews.apache.org/r/57110/#comment239759>
Per our offline discussion, looks like this belongs in one of the previous reviews that addresses the addition of the FrameworkInfo into the message? Seems like we need to send it always now that it includes the info.
src/master/master.cpp
Lines 6085-6091 (patched)
<https://reviews.apache.org/r/57110/#comment239761>
This could be simplified to:
```
const set<string> removedRoles = oldRoles - newRoles;
```
If we were to add the `-` operator to the existing operators in stout/set.hpp.
src/master/master.cpp
Lines 6116-6118 (patched)
<https://reviews.apache.org/r/57110/#comment239763>
And we'll expose this via metrics right? Maybe just say that we'll still account for the allocation to the role for the framework?
src/master/master.cpp
Lines 6124-6130 (patched)
<https://reviews.apache.org/r/57110/#comment239762>
Ditto here, could be:
```
const set<string> addedRoles = newRoles - oldRoles;
```
src/master/master.cpp
Lines 6133-6136 (patched)
<https://reviews.apache.org/r/57110/#comment239764>
How about a newline between the comment blocks for readability?
```
// We only add the framework to the role if it doesn't already exist.
//
// NOTE: It's possible that the framework already exists in the role since
// we don't remove the framework from the role until there are no resources
// being used by the framework within that role.
```
Ditto on the other notes.
src/master/master.cpp
Line 7453 (original), 7568 (patched)
<https://reviews.apache.org/r/57110/#comment239743>
Ditto here, this can just be:
```
foreach (const string& role, framework->roles) {
// ...
}
```
src/master/master.cpp
Lines 7453-7460 (original), 7568-7576 (patched)
<https://reviews.apache.org/r/57110/#comment239749>
Ditto here, I guess this case needs to be called from the constructor? Wonder if we need a `Framework::initialize`?
src/master/master.cpp
Line 7549 (original), 7656 (patched)
<https://reviews.apache.org/r/57110/#comment239738>
Should this be composing the message?
```
return Error("Failed to update framework: " + updateFramework_.error());
```
src/master/master.cpp
Lines 7892-7893 (original), 7992-7993 (patched)
<https://reviews.apache.org/r/57110/#comment239739>
No need to call the helper since we already maintain the set within the Framework struct. This can just be:
```
foreach (const string& role, framework->roles) {
removeFrameworkFromRole(framework, role);
}
```
src/master/master.cpp
Lines 7892-7894 (original), 7992-7994 (patched)
<https://reviews.apache.org/r/57110/#comment239747>
Currently, these functions are sometimes called from the Framework struct and sometimes from the Master, so the responsibilities between the master and the framework seem a little unclear, and potentially error prone.
Is it possible to place all of the calls into the role management into the framework struct? From our conversation offline, it sounds like we'll need to introduce a function like `Framework::complete` so that we can avoid calling `removeFrameworkFromRole` from the Framework destructor (which doesn't work since we keep completed frameworks).
- Benjamin Mahler
On Feb. 27, 2017, 10:19 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 10:19 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/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e
> src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942
>
>
> Diff: https://reviews.apache.org/r/57110/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>