----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 -----------------------------------------------------------
src/slave/containerizer/docker.cpp (line 1216) <https://reviews.apache.org/r/36326/#comment144687> let's spell out cgStats to cgroupStats src/slave/containerizer/docker.cpp (line 1219) <https://reviews.apache.org/r/36326/#comment144688> "Error getting cgroups statistics for container '" + stringify(containerId) + "': " + cgroupStats.error()" And why the extra LOG(WARNING) here? src/slave/containerizer/docker.cpp (line 1254) <https://reviews.apache.org/r/36326/#comment144689> 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. src/slave/containerizer/docker.cpp (line 1260) <https://reviews.apache.org/r/36326/#comment144690> 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 src/slave/containerizer/docker.cpp (line 1302) <https://reviews.apache.org/r/36326/#comment144692> Although it seems obvious from the right hand side you're getting the stat of cgroup, it wasn't obvious what type are you getting back, and more so about it's actually a Try<Stat> If you look at our style guide we only try to use auto in places the right hand side completely capture the type information. I think we should spell out the type here. src/slave/containerizer/docker.cpp (line 1327) <https://reviews.apache.org/r/36326/#comment144694> This seems too verbose being LOG(INFO). And what value do you think we have here logging this? - Timothy Chen 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 > >
