mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 36326: containerizer: added cgroups based statistics.
Date Fri, 10 Jul 2015 21:50:01 GMT


> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1266
> > <https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266>
> >
> >     I think you reordered things here that used to be in _usage.
> >     
> >     We used to first check the containers map, then check is it destroying, that
look at the pid and finally passing that on.
> >     
> >     Here the order is now pid check, and some reason skip the map check to get it
but then checking it later on.
> >     
> >     I think we should go back to the exact same order as before unless you have
reasons to do this change.

The thought was as follows:
- We called inspect to get the pid. So first we check for a pid.  Then we set the pid for
the container (if the container exists). Then we call "collectUsage" to collect stats.
- The check for DESTROYING is already being centralized in the collectUsage lambda.


> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1272
> > <https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1272>
> >
> >     If you like to a leave a comment why we called this function we should probably
comment at the place we call it not in the function (before docker->inspect).
> >     
> >     also s/didnt/didn't/g

Wanted to emphasize the logic of why we need to update the pid there (and not say in collectUSage).


> On July 10, 2015, 8:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 1342
> > <https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1342>
> >
> >     This seems too verbose being LOG(INFO).
> >     And what value do you think we have here logging this?

I can change it to LOG(DEBUG). Wanted to log there to help us debug.


- Jojy


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


On July 9, 2015, 8:38 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36326/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2951
>     https://issues.apache.org/jira/browse/MESOS-2951
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> WIP
> 
> Adding cgroups cpustat and memory statistics as preferred way to get usage
> statistics for containerizers.
> 
> Jira: MESOS-2951
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36326/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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