Re: Review Request 35118: Made updateSlave() update its 'totalResources'.
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::vectorResource 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'.
--- 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::vectorResource 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'.
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::vectorResource 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'.
--- 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::vectorResource 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'.
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::vectorResource 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'.
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::vectorResource 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'.
--- 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::vectorResource 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
