mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Guo <guojian...@cn.ibm.com>
Subject Re: Review Request 45014: Add /containers endpoint to return ResourceUsage.
Date Fri, 25 Mar 2016 06:30:27 GMT


> On March 22, 2016, 5:39 a.m., Jie Yu wrote:
> > Instead of handling /containers endpoint in Slave, I would suggest we dispatch the
request to ResourceMonitor. I will rename ResourceMonitor to ContainerMonitor, and we will
be deprecating the /monitor/statistics endpoints in the future.
> 
> Jay Guo wrote:
>     Thanks for reviewing! If I understand correctly, the /containers endpoint itself
is still `installed in slave`, although the action is `dispatched to` ResourceMonitorProcess
(aka ContainerMonitor) `via` ResourceMonitor (aka ContainerMonitor). We will make corresponding
changes if this is what you have in mind.
> 
> Jay Guo wrote:
>     *typo: ResourceMonitorProcess (aka ContainerMonitorProcess)
> 
> Jie Yu wrote:
>     Yes, this is what I was suggesting! Thanks!

Another question: Can we refactor current `ResourceMonitorProcess` to use `containerizer`
to collect `ResourceUsage/ContainerStatus`?

At present, collection of `ContainerStatus` would delegated back and forth between MonitorProcess
and Slave (Slave -> ResourceMonitor -> ResourceMonitorProcess -> Slave), which, IMHO,
is neither intuitive nor efficient. If ResourceMonitorProcess could possess a `containerizer`,
slave could simply dispatch the job to ResourceMonitorProcess with a list of ContainerID.

I'm thinking to refactor ResourceMonitorProcess constructor to something like:
```cpp
  explicit ResourceMonitorProcess(
      Containerizer* _containerizer,
      const lambda::function<list<ContainerID>()>& _containerIds)
    : ProcessBase("monitor"),
      containerizer(_containerizer),
      containerIds(_containerIds),
      limiter(2, Seconds(1)) {} // 2 permits per second.
```

In this case, `ResourceMonitorProcess::statistics` consults slave for `list<ContainerID>
`ResourceMonitorProcess::containerStatus` is invoked by slave with `list<ContainerID>

This requires modification of several test cases. If you agree, we will submit a seperate
patch of refactor.

Thanks!!


- Jay


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


On March 18, 2016, 5:27 a.m., Jay Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45014/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 5:27 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add /containers endpoint to return ResourceUsage.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/45014/diff/
> 
> 
> Testing
> -------
> 
> make check
> `curl agent_ip:port/containers` returns same json content as `curl agent_ip:port/monitor/statistics.json`
> 
> This is a draft patch of adding /containers endpoint to agent [MESOS-4891](https://issues.apache.org/jira/browse/MESOS-4891)
> 
> ContainerStatus will be added to response based on this patch.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>


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