You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/04/17 01:13:02 UTC
Re: Review Request 20339: Added Timer metric type.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 16, 2014, 4:13 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
switched from Mutex to internal::acquire.
Summary (updated)
-----------------
Added Timer metric type.
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION
3rdparty/libprocess/src/tests/metrics_tests.cpp 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
Diff: https://reviews.apache.org/r/20339/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20339: Added Timer metric type.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40722
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20339]
All tests passed.
- Mesos ReviewBot
On April 17, 2014, 10:41 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 10:41 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 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
>
> Diff: https://reviews.apache.org/r/20339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40822
-----------------------------------------------------------
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment73996>
s/std::string()/""/ here and below?
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment73994>
Can you add braces for the locked section and fix the indentation in this file? It will be a bit clunkier but, as you mentioned, we can add an RAII Lock (like the one in src/common/lock.hpp) later for our custom spinlock. The plan was to clean up our libprocess lock usage all at once.
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment73995>
s/Nanoseconds/Duration/
- Ben Mahler
On April 17, 2014, 10:41 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 10:41 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 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
>
> Diff: https://reviews.apache.org/r/20339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40850
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20339]
All tests passed.
- Mesos ReviewBot
On April 19, 2014, 12:47 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 19, 2014, 12:47 a.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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
> 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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 22, 2014, 4:57 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/timer.hpp, line 76
> > <https://reviews.apache.org/r/20339/diff/8/?file=564628#file564628line76>
> >
> > Do you need to push in the locked section?
i need to get in the locked section. Otherwise some other thread could change lastValue before the push happens.
> On April 22, 2014, 4:57 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, lines 139-172
> > <https://reviews.apache.org/r/20339/diff/8/?file=564629#file564629line139>
> >
> > It looks like we could get away with a simpler test that uses 1 timer and Clock::now to ensure the relative timing is correct.
I tried using Clock and it seemed that the stopwatch underlying timer didn't respect Clock changes. I may have done something wrong :)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review41051
-----------------------------------------------------------
On April 22, 2014, 11:35 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 11:35 a.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 0a8a31bf107041e4dd014f81785ac27e255f29a2
> 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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
> On April 22, 2014, 11:57 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/timer.hpp, line 76
> > <https://reviews.apache.org/r/20339/diff/8/?file=564628#file564628line76>
> >
> > Do you need to push in the locked section?
>
> Dominic Hamon wrote:
> i need to get in the locked section. Otherwise some other thread could change lastValue before the push happens.
Doesn't counter have the same issue but we've deemed it to be ok because it's racing anyway?
> On April 22, 2014, 11:57 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, lines 139-172
> > <https://reviews.apache.org/r/20339/diff/8/?file=564629#file564629line139>
> >
> > It looks like we could get away with a simpler test that uses 1 timer and Clock::now to ensure the relative timing is correct.
>
> Dominic Hamon wrote:
> I tried using Clock and it seemed that the stopwatch underlying timer didn't respect Clock changes. I may have done something wrong :)
Yeah I updated this to use Clock, let me know if you see any issues!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review41051
-----------------------------------------------------------
On April 22, 2014, 6:35 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 6:35 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 0a8a31bf107041e4dd014f81785ac27e255f29a2
> 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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review41051
-----------------------------------------------------------
Ship it!
Alright, I'll get this committed! I made a number of comments for cleanup below but I didn't open them as "issues" since I've included the fixes in the commit.
Please let me know if anything was missed or if we should follow up with anything!
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment74480>
No periods in these strings, how about "No value"?
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment74481>
Do we want it to be an error though? Let's kill this TODO since it deals with the potential implementation detail, and consider a TODO that motivates making these errors instead.
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment74444>
Do you need to push in the locked section?
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74435>
We can just use the process namespace as done in other tests!
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74423>
Typically we don't have using clauses for something as generic as "add" and "remove", it makes the tests here a bit less clear.
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74440>
In general let's try to prefer meaningful names like "counter" instead of "c", "timer" instead of "t", and "gauge" instead of "g".
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74437>
newline would have been nice here
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74438>
ditto here
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74424>
Missing include for duration.hpp.
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74441>
Seems a bit cleaner to just re-use one gauge here via assignment, I'll update the test.
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74432>
I'll update this to be s/MetricsTest/Metrics/ in this file.
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20339/#comment74433>
It looks like we could get away with a simpler test that uses 1 timer and Clock::now to ensure the relative timing is correct.
- Ben Mahler
On April 22, 2014, 6:35 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 6:35 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 0a8a31bf107041e4dd014f81785ac27e255f29a2
> 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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 22, 2014, 11:35 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
simplify Timer for round 1
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 0a8a31bf107041e4dd014f81785ac27e255f29a2
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
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
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.
Changes
-------
specify milliseconds.
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
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
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 21, 2014, 12:52 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
better braces to deal with lock scope.
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
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
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 18, 2014, 6:03 p.m., Ben Mahler wrote:
> > One tricky thing here is that the unit is implicit:
> >
> > Timer t("read");
> >
> > Now, we're exposing "read/..." with implicit units of nanoseconds. Anyone looking at the JSON will have a hard time figuring out what these values mean.
> >
> > Would it be better to implicitly append a unit (like "_secs") and expose seconds? Or, should we overload Timer constructors to take in different Duration subclasses to allow callers to customize the time units?
i like that idea, but i'm not sure how it would look. A templated constructor that passes the duration type to Timer::Data only goes so far - I'd have to track the units when pulling the data from Stopwatch::elapsed and convert before storing.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40841
-----------------------------------------------------------
On April 18, 2014, 5:47 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 5:47 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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40841
-----------------------------------------------------------
One tricky thing here is that the unit is implicit:
Timer t("read");
Now, we're exposing "read/..." with implicit units of nanoseconds. Anyone looking at the JSON will have a hard time figuring out what these values mean.
Would it be better to implicitly append a unit (like "_secs") and expose seconds? Or, should we overload Timer constructors to take in different Duration subclasses to allow callers to customize the time units?
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment74057>
Did you miss this one for adding braces?
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment74058>
Yikes, in these blocks let's try not to release the lock. You can do this by storing a Try<Nothing> result at the top and assigning it accordingly in the locked section. Ditto below.
- Ben Mahler
On April 19, 2014, 12:47 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 19, 2014, 12:47 a.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
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 18, 2014, 5:47 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased to master, added history hookup, and responded to benm's comments.
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
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
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 17, 2014, 3:41 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am d707ad759dacd16e0177e14f1bf5ece9e4ce2491
3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION
3rdparty/libprocess/src/tests/metrics_tests.cpp 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
Diff: https://reviews.apache.org/r/20339/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On April 17, 2014, 3:13 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/timer.hpp, lines 28-30
> > <https://reviews.apache.org/r/20339/diff/3/?file=561641#file561641line28>
> >
> > Why do these two return Futures? That may make this difficult to use since any time you want to time something you need to use a continuation to proceed?
> >
> > We may also want to comment on what the 'key' signifies. I imagine that many use-cases do not need a key and hence would it be useful to have a default key of "" along with a note that the key is only for timing operations in a concurrent manner?
the future was a hangover from the Mutex approach. Another reason not to use it :)
also added a comment and a default value.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40703
-----------------------------------------------------------
On April 17, 2014, 3:41 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 3:41 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 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
>
> Diff: https://reviews.apache.org/r/20339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40703
-----------------------------------------------------------
3rdparty/libprocess/include/process/metrics/timer.hpp
<https://reviews.apache.org/r/20339/#comment73811>
Why do these two return Futures? That may make this difficult to use since any time you want to time something you need to use a continuation to proceed?
We may also want to comment on what the 'key' signifies. I imagine that many use-cases do not need a key and hence would it be useful to have a default key of "" along with a note that the key is only for timing operations in a concurrent manner?
- Ben Mahler
On April 17, 2014, 4:10 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 4:10 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 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
>
> Diff: https://reviews.apache.org/r/20339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 17, 2014, 9:10 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added missing reference to new header
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am d707ad759dacd16e0177e14f1bf5ece9e4ce2491
3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION
3rdparty/libprocess/src/tests/metrics_tests.cpp 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
Diff: https://reviews.apache.org/r/20339/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20339: Added Timer metric type.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/#review40657
-----------------------------------------------------------
Bad patch!
Reviews applied: [20339]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
../../../3rdparty/libprocess/src/tests/metrics_tests.cpp:12:37: fatal error: process/metrics/timer.hpp: No such file or directory
compilation terminated.
make[6]: *** [tests-metrics_tests.o] Error 1
make[6]: *** Waiting for unfinished jobs....
make[5]: *** [check-am] Error 2
make[4]: *** [check-recursive] Error 1
make[3]: *** [check-recursive] Error 1
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On April 16, 2014, 11:17 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20339/
> -----------------------------------------------------------
>
> (Updated April 16, 2014, 11:17 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/include/process/metrics/timer.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
>
> Diff: https://reviews.apache.org/r/20339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20339: Added Timer metric type.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20339/
-----------------------------------------------------------
(Updated April 16, 2014, 4:17 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added jira link
Bugs: MESOS-1217
https://issues.apache.org/jira/browse/MESOS-1217
Repository: mesos-git
Description
-------
see summary.
Diffs
-----
3rdparty/libprocess/include/process/metrics/timer.hpp PRE-CREATION
3rdparty/libprocess/src/tests/metrics_tests.cpp 0cc9f4bcbbb03ac3a9a2d57f64b944443fcb94bb
Diff: https://reviews.apache.org/r/20339/diff/
Testing
-------
make check
Thanks,
Dominic Hamon