mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 35260: Refactored the ResourceMonitor to get statistics from the Slave.
Date Tue, 09 Jun 2015 23:49:26 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message