Re: 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/#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.
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.
--- 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.
--- 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.
--- 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
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
