You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/03/12 06:04:37 UTC
Review Request 19090: Enhanced DRF by including past allocations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-1086
https://issues.apache.org/jira/browse/MESOS-1086
Repository: mesos-git
Description
-------
See bug for more description.
Diffs
-----
src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
Diff: https://reviews.apache.org/r/19090/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 12, 2014, 4:07 p.m., Dominic Hamon wrote:
> > src/master/drf_sorter.cpp, line 106
> > <https://reviews.apache.org/r/19090/diff/1/?file=517048#file517048line106>
> >
> > client.allocations = it->allocations + 1;
I wanted this to be clearly two steps: (1) copy 'Client' and (2) increment 'allocations'.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review36923
-----------------------------------------------------------
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review36923
-----------------------------------------------------------
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68109>
style question for my edification: why not
client.name = it->name;
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68110>
client.allocations = it->allocations + 1;
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68111>
a vector would be more cache friendly for iteration, and would allow you to reserve the size of clients here to avoid allocations in the loop below.
- Dominic Hamon
On March 11, 2014, 10:04 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 10:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 14, 2014, 8:15 p.m., Ben Mahler wrote:
> > src/master/drf_sorter.cpp, lines 103-106
> > <https://reviews.apache.org/r/19090/diff/1/?file=517048#file517048line103>
> >
> > Can you copy construct here?
Great idea! I've fixed everywhere else too.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review37267
-----------------------------------------------------------
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review37267
-----------------------------------------------------------
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68691>
Can you copy construct here?
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68690>
Can you just copy construct here?
- Ben Mahler
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 26, 2014, 10:15 p.m., Adam B wrote:
> > src/master/drf_sorter.hpp, line 41
> > <https://reviews.apache.org/r/19090/diff/1/?file=517047#file517047line41>
> >
> > # of times since when? Since master/framework was (re)started? Or would/could this get recovered on master failover? This gets reset on framework restart/failover.
> > Would we prefer a rolling window of "# of times client has been chosen for allocation in the last X minutes"?
I've updated the comment to explain the field in more detail, thanks for your comment!
> On March 26, 2014, 10:15 p.m., Adam B wrote:
> > src/master/drf_sorter.cpp, line 80
> > <https://reviews.apache.org/r/19090/diff/1/?file=517048#file517048line80>
> >
> > Do we really want to reset allocations on framework deactivate/reactivate? Would we rather save the allocations value and revive it on reactivate?
> > Otherwise, a network disconnect or framework restart/failover will allow a framework to reset its allocations and move to the top of the list (but only if shares are already equal). Not a big deal.
Ideally we don't reset the allocations, but the way the code currently works that's what we do. I've added a comment to address this in the future.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review37343
-----------------------------------------------------------
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review37343
-----------------------------------------------------------
Ship it!
Looks good. Just a couple of questions/suggestions.
src/master/drf_sorter.hpp
<https://reviews.apache.org/r/19090/#comment68865>
# of times since when? Since master/framework was (re)started? Or would/could this get recovered on master failover? This gets reset on framework restart/failover.
Would we prefer a rolling window of "# of times client has been chosen for allocation in the last X minutes"?
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68866>
Do we really want to reset allocations on framework deactivate/reactivate? Would we rather save the allocations value and revive it on reactivate?
Otherwise, a network disconnect or framework restart/failover will allow a framework to reset its allocations and move to the top of the list (but only if shares are already equal). Not a big deal.
- Adam B
On March 11, 2014, 10:04 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 10:04 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 12, 2014, 5:47 a.m., Vinod Kone wrote:
> > Can you add a test for this?
I added a TODO in allocator_tests.cpp for now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review36903
-----------------------------------------------------------
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review36903
-----------------------------------------------------------
Can you add a test for this?
src/master/drf_sorter.hpp
<https://reviews.apache.org/r/19090/#comment68094>
s/choosen/chosen/
src/master/drf_sorter.cpp
<https://reviews.apache.org/r/19090/#comment68095>
Does this value grow unbounded? Its probably ok because even with a 1s allocation rate it is going to take lots of years before it wraps around :)
- Vinod Kone
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review37283
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19090: Enhanced DRF by including past allocations.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19090/#review36910
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19090]
All tests passed.
- Mesos ReviewBot
On March 12, 2014, 5:04 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19090/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 5:04 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1086
> https://issues.apache.org/jira/browse/MESOS-1086
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See bug for more description.
>
>
> Diffs
> -----
>
> src/master/drf_sorter.hpp f3fab71424fa10f87c87b733cc678fd90579a814
> src/master/drf_sorter.cpp bbf8270603e26a1b63e0f96bd5aa9c4ee4eb6d25
>
> Diff: https://reviews.apache.org/r/19090/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>