-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/#review90510
-----------------------------------------------------------


Thanks Bartek! See my detailed comments.


include/mesos/mesos.proto (lines 671 - 672)
<https://reviews.apache.org/r/36204/#comment143629>

    I think the 'total' here should be slaveInfo's resources (with no meta 
data, from command line --resourecs flag) applying checkpointed resources (for 
dynamic reservation/persistent volumes).
    
    The rationale is that the resource estimator might want to know about the 
reservation/persisent volume information to make a more informed decision. For 
example, some resource estimator might not want to oversubscribe dynamically 
reserved resources.
    
    So, could you please call out that the 'total' here is after applying the 
checkpointed resources to account for dynamic reservation and persistent 
volumes.
    
    ALso, could you please add a new line above the comment? Thanks!



src/slave/slave.cpp (line 4379)
<https://reviews.apache.org/r/36204/#comment143630>

    Per my comments above, the logic here needs to be adjusted. We need to 
apply checkpointed resource to info.resources() here.
    
    Please refer to the following code:
    https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883


- Jie Yu


On July 6, 2015, 3:25 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> -----------------------------------------------------------
> 
> (Updated July 6, 2015, 3:25 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2933
>     https://issues.apache.org/jira/browse/MESOS-2933
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Pass slave's total resources to the ResourceEstimator and QoSController via 
> Slave::usage().
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 
> 
> Diff: https://reviews.apache.org/r/36204/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Reply via email to