> On April 22, 2014, 1:04 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/include/process/metrics/timer.hpp, line 56 > > <https://reviews.apache.org/r/20339/diff/7/?file=563825#file563825line56> > > > > 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.
SGTM - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20339/#review41006 ----------------------------------------------------------- On April 21, 2014, 3: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, 3: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 > >
