----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35260/#review87270 -----------------------------------------------------------
Would be nice to split apart this diff to make it easier to review thoroughly. For example, the following seem to fit into separate patches: (1) Adding Slave::usage for resource estimators. (2) Updating the monitor to use Slave::usage. (3) Moving ResourceMonitorProcess into .cpp file. Also curious if we can use a lambda for the '`Slave::usage`' continuation? include/mesos/mesos.proto <https://reviews.apache.org/r/35260/#comment139582> Remove this comment? include/mesos/mesos.proto <https://reviews.apache.org/r/35260/#comment139583> This seems pretty close to what the source says ('allocated' Resources can be inferred), how about saying that this includes the resources for its active tasks? include/mesos/mesos.proto <https://reviews.apache.org/r/35260/#comment139584> Which isolation module? :) I think this comment was copied over from when we had a single isolator (CgroupsIsolator) and no containerizer. How about updating it to say "containerizer" instead? src/slave/monitor.hpp <https://reviews.apache.org/r/35260/#comment139585> Feel free to kill my TODO here and let's update the comment: ``` // Exposes resources usage information via a JSON endpoint. ``` src/slave/slave.cpp <https://reviews.apache.org/r/35260/#comment139657> Does this need to be a member function? Looks like you're feeding in everything you need. Is it possible to use a lambda for this? ``` { ResourceUsage usage; list<Future<ResourceStatistics>> futures; foreachvalue (const Framework* framework, frameworks) { foreachvalue (const Executor* executor, framework->executors) { ResourceUsage::Executor* entry = usage.add_executor(); entry->mutable_executor_info()->CopyFrom(executor->info); entry->mutable_allocated()->CopyFrom(executor->resources); futures.push_back(containerizer->usage(executor->containerId)); } } return await(futures) .then([&usage, &futures]() { size_t i = 0; foreach (const Future<ResourceStatistics> statistics, futures) { if (future.isReady() { usage.mutable_executor(i++)->mutable_statistics()->CopyFrom(future.get()); } else { LOG(WARNING) << ...; } } return usage; }); ``` Might be able to cut out a lot of the boilerplate? - Ben Mahler On June 9, 2015, 6:54 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35260/ > ----------------------------------------------------------- > > (Updated June 9, 2015, 6:54 p.m.) > > > Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-2818 > https://issues.apache.org/jira/browse/MESOS-2818 > > > Repository: mesos > > > Description > ------- > > Refactored the ResourceMonitor to get statistics from the Slave. > > 1) Modified ResourceUsage to include allocation information (see ticket for > reaons). > 2) Simplied the ResourceMonitor by taking a lambda from the slave to get > statistics. > 3) Adjusted (and simplified) the MonitorTest accordingly. > > > Diffs > ----- > > include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87 > include/mesos/slave/resource_estimator.hpp > 7f78fd86760397d5b885a2c00b41081d0b546cdd > src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7 > src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c > src/slave/resource_estimators/fixed.cpp > 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 > src/slave/resource_estimators/noop.hpp > 7a44e6deefc9c1985c14d076a427aa5c654aa1bb > src/slave/resource_estimators/noop.cpp > 5f135ff37e84c248ede29bbe4a7d1b8319417e20 > src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e > src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac > src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 > src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a > src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 > > Diff: https://reviews.apache.org/r/35260/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
