----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20339/#review41006 -----------------------------------------------------------
Looks good, I have a suggestion below to simplify Timer usage for callers and for the short term we can kill 'keys' and leave my suggestion below as a TODO, let me know what you think! 3rdparty/libprocess/include/process/metrics/timer.hpp <https://reviews.apache.org/r/20339/#comment74359> 'key' is less for allowing concurrent calls to start, and more for supporting the timing of concurrent operations, let's clarify the comment to better guide the reader here? 3rdparty/libprocess/include/process/metrics/timer.hpp <https://reviews.apache.org/r/20339/#comment74360> What if the caller wishes to retry an operation is retried for the same key? Are they expected to stop the Timer even though the operation did not complete? We're also implicitly saying here that not calling 'stop' is an error as it will "leak" memory, what pattern do you suggest to callers for stopping Timers? timer.start(key); async(operation) .onAny(lambda::bind(Timer::stop, timer, key) .then(defer(self(), &Self::_operation)); These caveats seem fairly unfortunate. I'm wondering if it's cleaner to support the two use cases separately: timer.start()/timer.stop() for non-concurrent operations timer.time(future) for concurrent operations (which internally uses a unique key in a map) This way, one could write: // Time first half. timer.time(async(operation)) .then(defer(self(), &Self::_operation)); // Time both halves. timer.time( async(operation) .then(defer(self(), &Self::_operation)) ); // Time bottom half. async(operation) .then(timer.time(defer(self(), &Self::_operation))); For time, I would recommend we remove the keys here and leave TODOs for Timer.time(). As long as we only time operations inside the Registrar we won't need to support the concurrent operations in the short term and we can follow up here separately. - Ben Mahler On April 21, 2014, 10:43 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20339/ > ----------------------------------------------------------- > > (Updated April 21, 2014, 10:43 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1217 > https://issues.apache.org/jira/browse/MESOS-1217 > > > Repository: mesos-git > > > Description > ------- > > see summary. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am d707ad759dacd16e0177e14f1bf5ece9e4ce2491 > 3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION > 3rdparty/libprocess/src/tests/metrics_tests.cpp > abe1588c931b45a09294812974788aa74de44dd4 > > Diff: https://reviews.apache.org/r/20339/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
