You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/05/07 00:35:31 UTC
Review Request 33918: Added resources estimator abstraction for
oversubscription.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2649
https://issues.apache.org/jira/browse/MESOS-2649
Repository: mesos
Description
-------
Added resources estimator abstraction for oversubscription.
This patch defines the interface of the resources estimator and creates a default stub implementation.
Diffs
-----
include/mesos/slave/resources_estimator.hpp PRE-CREATION
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/slave/resources_estimator.hpp PRE-CREATION
src/slave/resources_estimator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33918/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Jie Yu <yu...@gmail.com>.
> On May 7, 2015, 4:57 p.m., Niklas Nielsen wrote:
> > src/slave/resources_estimator.cpp, lines 59-60
> > <https://reviews.apache.org/r/33918/diff/1/?file=951653#file951653line59>
> >
> > Shouldn't we be able to start the process in the constructor and forward initialize() to the process here?
'initialize' will take a resource monitor pointer in the near future, and as far as I know, we don't support initialize a module instance with a pointer.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/#review82829
-----------------------------------------------------------
On May 6, 2015, 10:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33918/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 10:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2649
> https://issues.apache.org/jira/browse/MESOS-2649
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added resources estimator abstraction for oversubscription.
>
> This patch defines the interface of the resources estimator and creates a default stub implementation.
>
>
> Diffs
> -----
>
> include/mesos/slave/resources_estimator.hpp PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/slave/resources_estimator.hpp PRE-CREATION
> src/slave/resources_estimator.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33918/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/#review82829
-----------------------------------------------------------
Ship it!
Looks good! I agree with Vinod about the naming, but apart from that (and the comments below): Shipit!
include/mesos/slave/resources_estimator.hpp
<https://reviews.apache.org/r/33918/#comment133669>
A comment here about the component and how it fits in the whole system would be great :)
include/mesos/slave/resources_estimator.hpp
<https://reviews.apache.org/r/33918/#comment133670>
Nice!
src/slave/resources_estimator.cpp
<https://reviews.apache.org/r/33918/#comment133663>
How about adding a bit more context? For example: "Resource estimator not yet initialized"?
src/slave/resources_estimator.cpp
<https://reviews.apache.org/r/33918/#comment133664>
Shouldn't we be able to start the process in the constructor and forward initialize() to the process here?
src/slave/resources_estimator.cpp
<https://reviews.apache.org/r/33918/#comment133662>
Same here?
- Niklas Nielsen
On May 6, 2015, 3:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33918/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 3:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2649
> https://issues.apache.org/jira/browse/MESOS-2649
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added resources estimator abstraction for oversubscription.
>
> This patch defines the interface of the resources estimator and creates a default stub implementation.
>
>
> Diffs
> -----
>
> include/mesos/slave/resources_estimator.hpp PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/slave/resources_estimator.hpp PRE-CREATION
> src/slave/resources_estimator.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33918/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/
-----------------------------------------------------------
(Updated May 13, 2015, 10:35 p.m.)
Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Changes
-------
Renamed EmptyResourceEstimator to NoopResourceEstimator
Bugs: MESOS-2649
https://issues.apache.org/jira/browse/MESOS-2649
Repository: mesos
Description
-------
Added resources estimator abstraction for oversubscription.
This patch defines the interface of the resources estimator and creates a default stub implementation.
Diffs (updated)
-----
include/mesos/slave/resource_estimator.hpp PRE-CREATION
src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4
src/slave/resource_estimator.hpp PRE-CREATION
src/slave/resource_estimator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33918/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/
-----------------------------------------------------------
(Updated May 13, 2015, 12:23 a.m.)
Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Changes
-------
Updated the comments according to our new discussion.
s/MesosResourceEstimator/EmptyResourceEstimator/ per BenM's comments.
Bugs: MESOS-2649
https://issues.apache.org/jira/browse/MESOS-2649
Repository: mesos
Description
-------
Added resources estimator abstraction for oversubscription.
This patch defines the interface of the resources estimator and creates a default stub implementation.
Diffs (updated)
-----
include/mesos/slave/resource_estimator.hpp PRE-CREATION
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/slave/resource_estimator.hpp PRE-CREATION
src/slave/resource_estimator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33918/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/
-----------------------------------------------------------
(Updated May 8, 2015, 8:24 p.m.)
Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed review comments.
Bugs: MESOS-2649
https://issues.apache.org/jira/browse/MESOS-2649
Repository: mesos
Description
-------
Added resources estimator abstraction for oversubscription.
This patch defines the interface of the resources estimator and creates a default stub implementation.
Diffs (updated)
-----
include/mesos/slave/resource_estimator.hpp PRE-CREATION
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/slave/resource_estimator.hpp PRE-CREATION
src/slave/resource_estimator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/33918/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Jie Yu <yu...@gmail.com>.
> On May 7, 2015, 9:06 p.m., Ben Mahler wrote:
> > src/slave/resources_estimator.hpp, line 34
> > <https://reviews.apache.org/r/33918/diff/1/?file=951652#file951652line34>
> >
> > Interesting, what will go in here? I have a hard time seeing a general "Mesos"ResourcesEstimator evolve. It seems like there might be a number of different estimators included in Mesos that folks can choose from. For example, to disable oversubscription, we could just use a no-op estimator, yes? Maybe that's what we should implement here? An EmptyResourceEstimator, or NoopResourceEstimator?
> >
> > That seems like an easy way to disable oversubscription? (ignoring whether disabling means killing revocable containers)
> >
> > Overall this feels similar to how we have the Allocator interface, a DRF allocator implementation and potentially others (note we didn't call it the MesosAllocator :)). Containerizer is another case where we have a pluggable isolation based containerizer, a docker containerizer, and potentially others to choose from.
> >
> > Just some food for thought. :)
> I have a hard time seeing a general "Mesos"ResourcesEstimator evolve.
I think ultimately, we still need a _default_ resource estimator that is shipped with Mesos. I would imagine most of the users do not care about the prediction model and simply just want to enable oversubscription. What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/#review82905
-----------------------------------------------------------
On May 6, 2015, 10:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33918/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 10:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2649
> https://issues.apache.org/jira/browse/MESOS-2649
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added resources estimator abstraction for oversubscription.
>
> This patch defines the interface of the resources estimator and creates a default stub implementation.
>
>
> Diffs
> -----
>
> include/mesos/slave/resources_estimator.hpp PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/slave/resources_estimator.hpp PRE-CREATION
> src/slave/resources_estimator.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33918/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/#review82905
-----------------------------------------------------------
src/slave/resources_estimator.hpp
<https://reviews.apache.org/r/33918/#comment133740>
Interesting, what will go in here? I have a hard time seeing a general "Mesos"ResourcesEstimator evolve. It seems like there might be a number of different estimators included in Mesos that folks can choose from. For example, to disable oversubscription, we could just use a no-op estimator, yes? Maybe that's what we should implement here? An EmptyResourceEstimator, or NoopResourceEstimator?
That seems like an easy way to disable oversubscription? (ignoring whether disabling means killing revocable containers)
Overall this feels similar to how we have the Allocator interface, a DRF allocator implementation and potentially others (note we didn't call it the MesosAllocator :)). Containerizer is another case where we have a pluggable isolation based containerizer, a docker containerizer, and potentially others to choose from.
Just some food for thought. :)
- Ben Mahler
On May 6, 2015, 10:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33918/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 10:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2649
> https://issues.apache.org/jira/browse/MESOS-2649
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added resources estimator abstraction for oversubscription.
>
> This patch defines the interface of the resources estimator and creates a default stub implementation.
>
>
> Diffs
> -----
>
> include/mesos/slave/resources_estimator.hpp PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/slave/resources_estimator.hpp PRE-CREATION
> src/slave/resources_estimator.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33918/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 33918: Added resources estimator abstraction for
oversubscription.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33918/#review82760
-----------------------------------------------------------
LGTM except for the naming. I would call it ResourceEstimator instead of Resource*s*Estimator. The former is just easier to pronounce. Also, this is to be consistent with things like ResourceMonitor and StatusUpdateManager which also deal with multiples of resources and status updates.
- Vinod Kone
On May 6, 2015, 10:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33918/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 10:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2649
> https://issues.apache.org/jira/browse/MESOS-2649
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added resources estimator abstraction for oversubscription.
>
> This patch defines the interface of the resources estimator and creates a default stub implementation.
>
>
> Diffs
> -----
>
> include/mesos/slave/resources_estimator.hpp PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/slave/resources_estimator.hpp PRE-CREATION
> src/slave/resources_estimator.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33918/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>