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

2015-07-07 Thread Bartek Plotka 

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

2015-07-07 Thread Jie Yu 

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


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.

2015-07-07 Thread Mesos ReviewBot 

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

2015-07-07 Thread Bartek Plotka 


> On July 6, 2015, 5:50 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, line 4379
> > 
> >
> > 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.

2015-07-07 Thread Bartek Plotka 

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

2015-07-06 Thread Mesos ReviewBot 

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

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

2015-07-06 Thread Jie Yu 

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


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)


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