You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2015/06/05 23:09:16 UTC
Review Request 35118: Made updateSlave() update its 'totalResources'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-2776
https://issues.apache.org/jira/browse/MESOS-2776
Repository: mesos
Description
-------
- This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
- Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
Diffs
-----
src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
src/master/master.cpp be0db42da3c59761aa154439653d715556465256
Diff: https://reviews.apache.org/r/35118/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review87083
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 9:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Ben Mahler <be...@gmail.com>.
> On June 5, 2015, 9:58 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3462
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3462>
> >
> > woah. didn't realize this was handled automagically by the install handler.
Yeah, we didn't do this for framework provided input since the construction to 'Resources' is lossy, and we wanted to validate, but looks good here! Just good to keep in mind that doing this means we cannot do a `Resources::validate` on this.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review86850
-----------------------------------------------------------
On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 9:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 5, 2015, 2:58 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3517
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3517>
> >
> > while you are at it, can you just send slave->totalResources here instead of oversubscribedResources? this will address my TODO on this method.
The allocator expects only oversubscribedResources and does a CHECK on it. The logic in Allocator::updateSlave(...) needs a little tweaking as well. I'd be happy to do it in a subsequent review.
SG?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review86850
-----------------------------------------------------------
On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review86850
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/35118/#comment138949>
woah. didn't realize this was handled automagically by the install handler.
src/master/master.cpp
<https://reviews.apache.org/r/35118/#comment138951>
while you are at it, can you just send slave->totalResources here instead of oversubscribedResources? this will address my TODO on this method.
- Vinod Kone
On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 9:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Ben Mahler <be...@gmail.com>.
> On June 9, 2015, 5:36 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 3513-3514
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513>
> >
> > Does it make sense to point to some documentation (if it exists already, or inline) about how this resource math will work?
>
> Jiang Yan Xu wrote:
> Can you suggest something here?
>
> FWIW I also updated the comment on `totalResources`:
> ```
> // The current total resources of the slave. Note that this is
> // different from 'info.resources()' because this also considers
> // operations (e.g., CREATE, RESERVE) that have been applied and
> // includes revocable resources as well.
> Resources totalResources;
> ```
Given the check above, how about calling .revocable to make it symmetric:
```
slave->totalResources -= slave->totalResources.revocable();
slave->totalResources += oversubscribedResources.revocable();
```
Should be a bit clearer to the reader that what we're doing here is updating the revocable resources, without needing a big comment.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review87234
-----------------------------------------------------------
On June 5, 2015, 9:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 9:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 3511
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3511>
> >
> > Isn't this a bit harsh? Shouldn't we log a warning and return instead? Also, could this be done earlier on?
Harshness: I think it's fine a) because it's internal communication not involving the frameworks and b) it's a new message handler so backwards compatibility is covered.
Done earlier on: Agreed, will change this in a new review. (This one is already committed, sorry I haven't flagged it as such).
> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 3513-3514
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513>
> >
> > Does it make sense to point to some documentation (if it exists already, or inline) about how this resource math will work?
Can you suggest something here?
FWIW I also updated the comment on `totalResources`:
```
// The current total resources of the slave. Note that this is
// different from 'info.resources()' because this also considers
// operations (e.g., CREATE, RESERVE) that have been applied and
// includes revocable resources as well.
Resources totalResources;
```
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review87234
-----------------------------------------------------------
On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 3513-3514
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513>
> >
> > Does it make sense to point to some documentation (if it exists already, or inline) about how this resource math will work?
>
> Jiang Yan Xu wrote:
> Can you suggest something here?
>
> FWIW I also updated the comment on `totalResources`:
> ```
> // The current total resources of the slave. Note that this is
> // different from 'info.resources()' because this also considers
> // operations (e.g., CREATE, RESERVE) that have been applied and
> // includes revocable resources as well.
> Resources totalResources;
> ```
>
> Ben Mahler wrote:
> Given the check above, how about calling .revocable to make it symmetric:
>
> ```
> slave->totalResources -= slave->totalResources.revocable();
> slave->totalResources += oversubscribedResources.revocable();
> ```
>
> Should be a bit clearer to the reader that what we're doing here is updating the revocable resources, without needing a big comment.
SGTM. Will do.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review87234
-----------------------------------------------------------
On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 35118: Made updateSlave() update its
'totalResources'.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35118/#review87234
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/35118/#comment139552>
Isn't this a bit harsh? Shouldn't we log a warning and return instead? Also, could this be done earlier on?
src/master/master.cpp
<https://reviews.apache.org/r/35118/#comment139553>
Does it make sense to point to some documentation (if it exists already, or inline) about how this resource math will work?
- Niklas Nielsen
On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> -----------------------------------------------------------
>
> (Updated June 5, 2015, 2:09 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& oversubscribedResources` instead of `const std::vector<Resource>& oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`.
>
>
> Diffs
> -----
>
> src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f
> src/master/master.cpp be0db42da3c59761aa154439653d715556465256
>
> Diff: https://reviews.apache.org/r/35118/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>