----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21451/#review44533 -----------------------------------------------------------
Are there dependent reviews? src/slave/containerizer/isolators/cgroups/perf_event.hpp <https://reviews.apache.org/r/21451/#comment78951> Does this need to be a Future? It looks like you're never using a pending value. Should this be an Option or just a PerfStatistics if you want a default initial value? How about calling this perfStatistics? src/slave/containerizer/isolators/cgroups/perf_event.hpp <https://reviews.apache.org/r/21451/#comment78952> Why the removal of the newlines here? src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78953> No periods in Errors or Failures. src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78955> So one single RunState missing the container id causes everything to fail? Why is the container id an optional field? Chatting with Vinod it sounds like it was an artifact of UUID not having a default constructor. If we can make it non-optional that would clean this up. src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78954> Not for this review, but two comments: (1) CHECK_NOTNULL immediately after 'new' seems unnecessary. (2) Would it be cleaner to create 'info' *after* validating? Then you don't need to worry about deleting it below. src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78956> Per my comments above, how is this case possible? src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78958> This is pretty dangerous, in that we need to be very careful not to have an unbounded growth of perf samples running! It is much easier to get right if we trigger the next sample only *after* the current sample completes. You can still achieve the current semantics by using a .after timeout! src/slave/containerizer/isolators/cgroups/perf_event.cpp <https://reviews.apache.org/r/21451/#comment78959> Consider just storing the Option to avoid the .get() call here. - Ben Mahler On May 29, 2014, 10:14 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21451/ > ----------------------------------------------------------- > > (Updated May 29, 2014, 10:14 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Bugs: MESOS-1278 > https://issues.apache.org/jira/browse/MESOS-1278 > > > Repository: mesos-git > > > Description > ------- > > Three new slave flags: > 1. perf_events is a list of perf events to sample. This list is validated > through perf during isolation creation. > 2. perf_interval is the time between starting new samples > 3. perf_duration is the duration of each sample > > e.g., you could sample cpu-cycles and cpu-migrations for 10 seconds every 60 > seconds. > > Isolator::usage() will initially return an empty PerfStatistics protobuf > (containing only the timing information) until the first sample is obtained. > After that it will return the most recent sample. > > > Diffs > ----- > > src/slave/containerizer/isolators/cgroups/perf_event.hpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/perf_event.cpp PRE-CREATION > src/slave/containerizer/mesos_containerizer.cpp > 14380240512d29b49c2e8f2831a9ef8ca102a024 > src/slave/flags.hpp 15e5b64fd24a9381074b4833a0403314e1f404bc > src/tests/isolator_tests.cpp b0eff5721b6ea4dcee8ff3748a2a11ade809e30e > src/tests/mesos.cpp 3065ae30bceb8b258d071110c0360f701ff48b64 > src/tests/slave_recovery_tests.cpp 44ffac40b9edc9940f17b5fbe1848d56cf53b69b > > Diff: https://reviews.apache.org/r/21451/diff/ > > > Testing > ------- > > Test forthcoming. > > > Thanks, > > Ian Downes > >
