> On July 2, 2015, 11:32 p.m., Ben Mahler wrote: > > src/linux/cgroups.cpp, lines 2014-2027 > > <https://reviews.apache.org/r/36106/diff/1/?file=997647#file997647line2014> > > > > Any reason you're not just re-using cgroups::stat here? I'd suggest > > just calling cgroups::stat for now, should simplify this and make it easier > > for the reader. :) > > Jojy Varghese wrote: > The only reason being that the way cpuacct creates Stat should be > encapsulated in the cpuacct::Stat. This is the same reason there is a parse > method in Stat. But I can change it to use cgroups::stat if absolutely > necessary. > > Jojy Varghese wrote: > Also wanted to highlight the semantics of Stat class to make it clear > what the intent is. Stat can only be created from data read from the stats > file (cpuacct.stat). Thus creating Stat without the file data should not be > allowed. Once the Stat object is created, you can not mutate it. > > Ben Mahler wrote: > Imagine this only returned the user time, then we would return a > Duration, yes? We would not be returning a `Stat` class with one const 'user' > member, with a const getter and a static parse method. That would be > overkill, right? > > That's the reasoning I have for making this a plain struct. This is the > same situation as above, but we need to return more than one value (e.g. > `os::UTSInfo`, `os::Memory`, `os::Load`, etc.)
If we were to just return a single value, I would have preferred it to be a rvalue. Duration is a possiblity but not ideal in this situation as we need just the ticks to seconds convertion which is not provided by Duration. Structures like UTSInfo, Memory etc should be non-mutable as they represet a snapshot of data IMHO. - Jojy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 ----------------------------------------------------------- On July 2, 2015, 8:09 p.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36106/ > ----------------------------------------------------------- > > (Updated July 2, 2015, 8:09 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and > Timothy Chen. > > > Repository: mesos > > > Description > ------- > > cgroups implementation does not have a cpuacct subsystem implementation as of > today. Adding the implementation for stat function. > > Changes: > - added Stats class to encapsulate cpuacct.stat data > - added implementation for cpuacct::stats > - added unit tests > > Jira: MESOS-2961 > > > Diffs > ----- > > src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 > src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 > src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a > > Diff: https://reviews.apache.org/r/36106/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jojy Varghese > >
