You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bartek Plotka <bw...@gmail.com> on 2015/07/06 17:25:11 UTC

Review Request 36204: Pass slave's total resources in ResourceUsage.

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

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

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/
-----------------------------------------------------------

(Updated July 7, 2015, 5:50 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.


Changes
-------

Changed totalResources check.


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 (updated)
-----

  include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
  src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 

Diff: https://reviews.apache.org/r/36204/diff/


Testing
-------

make check.


Thanks,

Bartek Plotka

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/#review90727
-----------------------------------------------------------

Ship it!


LGTM! Could you please add a follow up patch to test this code path?


src/slave/slave.cpp (lines 4382 - 4383)
<https://reviews.apache.org/r/36204/#comment143863>

    You should just add a CHECK here:
    ```
    CHECK_SOME(totalResources)
      << "Failed to apply checkpointed resource "
      << checkpointedResources << " to slave's resources "
      << info->resources();
    
    usage->mutable_total()->CopyFrom(totalResources.get());
    ```


- Jie Yu


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 4:20 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
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/#review90722
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36204]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 4:20 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
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/
-----------------------------------------------------------

(Updated July 7, 2015, 4:20 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.


Changes
-------

Slave's total resources now includes checkpointed dynamic reservations and persistent volumes.


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 (updated)
-----

  include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
  src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 

Diff: https://reviews.apache.org/r/36204/diff/


Testing
-------

make check.


Thanks,

Bartek Plotka

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Bartek Plotka <bw...@gmail.com>.

> On July 6, 2015, 5:50 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, line 4379
> > <https://reviews.apache.org/r/36204/diff/1/?file=1000223#file1000223line4379>
> >
> >     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

Thanks Jie for your feedback! 

I addressed your issue, however I'm just curious if there is any possibility to get an error (here in *::usage()*) from *applyCheckpointedResources* function? (https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883). IMO here we are quite sure that *checkpointedResources* are proper. If yes, would it make sense to just put *info.resources()* into *usage->total* in case we cannot apply checkpointed resources?


- Bartek


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


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 4:20 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
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36204/#review90516
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36204]

All tests passed.

- Mesos ReviewBot


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
> 
>