> On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/Makefile.in, line 137 > > <https://reviews.apache.org/r/3050/diff/1/?file=62806#file62806line137> > > > > MONITORING_OBJ should be set to empty string unconditionally first.
Done. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.hpp, line 39 > > <https://reviews.apache.org/r/3050/diff/1/?file=62807#file62807line39> > > > > Please state whether RSS or VSIZE or something else. Done. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.cpp, line 22 > > <https://reviews.apache.org/r/3050/diff/1/?file=62808#file62808line22> > > > > include all system .h headers either before or after standard C++ > > headers. Done. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.cpp, line 61 > > <https://reviews.apache.org/r/3050/diff/1/?file=62808#file62808line61> > > > > Please make these file-scoped (static or anonymous namespace) or put > > them in the header file. Done. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.cpp, line 81 > > <https://reviews.apache.org/r/3050/diff/1/?file=62808#file62808line81> > > > > O is a pretty bad variable name. Done. I was using outdated documentation where this field was a hard-coded 0 in proc. It is now num_threads. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.cpp, line 85 > > <https://reviews.apache.org/r/3050/diff/1/?file=62808#file62808line85> > > > > Check whether this read succeeds. Done. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.hpp, line 37 > > <https://reviews.apache.org/r/3050/diff/1/?file=62807#file62807line37> > > > > Why milliseconds? libprocess uses seconds since some time, and I think > > USER_HZ usually (often?) isn't 1000. Sam and I decided on milliseconds (since epoch when appropriate) for all measured times because it was the granularity closest to the times in various sources, and having one unit used consistently seemed cleaner. Would it make it more consistent with the rest of Mesos if we scale it up to seconds? For reference, granularity of various times we used: process start time (used for duration of initial read)- jiffies (4ms +/- depending on system HZ) boot time (used for start time to make it since epoch)- seconds current time (used for duration) - nanoseconds/milliseconds since epoch (depends on system) cpu time (proc)- clock ticks (10ms +/- depending on system SC_CLK_TCK) cpu time (lxc)- nanoseconds (unit returned in, not sure of actual granularity) Period of measurement - potential lower bound on a single machine is probably in the millisecond range. > On 2011-12-07 21:10:13, Charles Reiss wrote: > > src/monitoring/proc_utils.hpp, line 33 > > <https://reviews.apache.org/r/3050/diff/1/?file=62807#file62807line33> > > > > Why strings? (and elsewhere) > > I was deciding between using unsigned integers, something like the pid_t type, and strings. I ultimately decided on strings because they supported special non-integer 'pids' proc lets you query (i.e. "self"). This was convenient for testing. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3050/#review3712 ----------------------------------------------------------- On 2011-12-08 00:00:32, Alex Degtiar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3050/ > ----------------------------------------------------------- > > (Updated 2011-12-08 00:00:32) > > > Review request for mesos. > > > Summary > ------- > > The first of several patches related to resource usage monitoring. This patch > provides a collection of utilities for use on Linux for reading stats from > proc. It is used by both the lxc and proc resource collectors. > > > This addresses bug MESOS-89. > https://issues.apache.org/jira/browse/MESOS-89 > > > Diffs > ----- > > src/tests/Makefile.in ea943f7 > src/tests/proc_utils_tests.cpp PRE-CREATION > src/monitoring/proc_utils.cpp PRE-CREATION > src/Makefile.in 516f128 > src/monitoring/proc_utils.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/3050/diff > > > Testing > ------- > > Sanity tests have been written in src/tests/proc_utils_tests.cpp for all > utility functions, and functions have been tested ad hoc. > > > Thanks, > > Alex > >
