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/05/02 00:50:00 UTC
Review Request 20995: Removed defer dependency from Gauge.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Removed defer dependency from Gauge.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42138
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20995]
All tests passed.
- Mesos ReviewBot
On May 2, 2014, 7:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 7:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42756
-----------------------------------------------------------
Looks good, just a small amount of cleanup and we can get this committed!
3rdparty/libprocess/include/process/metrics/gauge.hpp
<https://reviews.apache.org/r/20995/#comment76648>
Should we be including <stout/none.hpp> as well?
3rdparty/libprocess/include/process/metrics/gauge.hpp
<https://reviews.apache.org/r/20995/#comment76649>
Thanks for the comment!
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20995/#comment76643>
s/forty_two/fortyTwo/ or just make this 'one' or 'zero' to make this as simple as possible.
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20995/#comment76635>
Why this indirection? Can't this just return 42 directly?
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20995/#comment76639>
What is a "synchgauge"? :)
How about we clarify these Gauge names a bit:
test/function_gauge
test/deferred_gauge
test/failed_gauge
- Ben Mahler
On May 12, 2014, 7:37 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 7:37 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42766
-----------------------------------------------------------
Ship it!
I'll get this committed, thanks!
3rdparty/libprocess/src/tests/metrics_tests.cpp
<https://reviews.apache.org/r/20995/#comment76653>
As usual, two spaces between top-level functions. I'll make this change for you before committing.
- Ben Mahler
On May 12, 2014, 10:38 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 10:38 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 12, 2014, 3:38 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs (updated)
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 12, 2014, 12:37 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs (updated)
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 12, 2014, 12:29 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
formatting tweak. thanks, mesos-style!
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ccf80a771c59092cb72d4dac0d868eec7df2f088
3rdparty/libprocess/3rdparty/stout/tests/proc_tests.cpp c723bd769b2a698e88a1c46c5b669c35ff1fb7ac
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 06511858dde89fc74ef5826951dfa474c630bc97
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803e42284f641879fb24bce1282215c8ea52
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084661345f9367c75f9db61279f032b93e69
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d39e9588eb8eea6d739451ad3bab029ca08
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
src/slave/slave.hpp a6efad4763a7180daad63689227b46e196e5e1a9
src/slave/slave.cpp 3a4ae38e06a1c23daafebf5421d996e649a07ca5
src/tests/master_tests.cpp 30ea7ffad4eb0ad011942d91cbf61284b031a80e
src/tests/slave_tests.cpp 458356d56522a02222f6fd6f2cf9927b318beda2
support/post-reviews.py 0ba14d8947cd7004a939c8b9b95deaab035be928
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42724
-----------------------------------------------------------
Bad patch!
Reviews applied: [20995]
Failed command: ./support/mesos-style.py
Error:
Checking 428 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+runtime/vlog,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/authorizer/authorizer.hpp:330: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]
3rdparty/libprocess/include/process/metrics/gauge.hpp:26: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]
Total errors found: 2
- Mesos ReviewBot
On May 8, 2014, 11:28 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 11:28 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Allow Gauge to take simple functions.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 8, 2014, 4:28 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Added second constructor instead of replacing it.
Summary (updated)
-----------------
Allow Gauge to take simple functions.
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs (updated)
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 8a131218b858a6571bd520c470eac850a3fd345e
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Removed defer dependency from Gauge.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 7, 2014, 3:55 p.m., Ben Mahler wrote:
> > I thought we were going to leave the 'deferred' constructor (asynchronous) and provide an overload for a function<double> (synchronous) constructor? With the function<double> constructor, we need to use async() to wrap the function and ensure we don't block MetricsProcess.
> >
> > If we were to expose function<Future<double> as currently done, then it's not possible to tell if the function is actually asynchronous, and we'd have to always wrap it with 'async()' to ensure we don't block, no? For asynchronous functions the unneeded wrapping seems unfortunate.
You're absolutely right - but we have different assumptions. You are assuming that we always want Gauge to be asynchronous, I am expecting there are cases when it is reasonable to not be. For example, when getting the queue size from the Master. My expectation:
constructed with a std::function - function call is synchronous and potentially blocks MetricsProcess.
constructed with a Deferred - function call is asynchronous and doesn't block MetricsProcess.
I understand this means that the user can potentially block MetricsProcess by not deferring, however it means we don't incur the overhead of process::async just to call a simple method. If you think this is too fragile, then yes, we'd have to have two overloads (or maybe two Gauge types: Gauge and DeferredGauge). We could then store Options in the shared data structure and decide whether to use async depending on which is set.
Perhaps the two types is cleaner?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42435
-----------------------------------------------------------
On May 2, 2014, 12:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 12:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Removed defer dependency from Gauge.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/#review42435
-----------------------------------------------------------
I thought we were going to leave the 'deferred' constructor (asynchronous) and provide an overload for a function<double> (synchronous) constructor? With the function<double> constructor, we need to use async() to wrap the function and ensure we don't block MetricsProcess.
If we were to expose function<Future<double> as currently done, then it's not possible to tell if the function is actually asynchronous, and we'd have to always wrap it with 'async()' to ensure we don't block, no? For asynchronous functions the unneeded wrapping seems unfortunate.
- Ben Mahler
On May 2, 2014, 7:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20995/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 7:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1286
> https://issues.apache.org/jira/browse/MESOS-1286
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary.
>
> This allows synchronous Gauge methods which reduces potential process queue pressure.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
> 3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
>
> Diff: https://reviews.apache.org/r/20995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20995: Removed defer dependency from Gauge.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 2, 2014, 12:52 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
added TODO
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs (updated)
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 20995: Removed defer dependency from Gauge.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20995/
-----------------------------------------------------------
(Updated May 2, 2014, 10:56 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
added issue ref
Bugs: MESOS-1286
https://issues.apache.org/jira/browse/MESOS-1286
Repository: mesos-git
Description
-------
see summary.
This allows synchronous Gauge methods which reduces potential process queue pressure.
Diffs
-----
3rdparty/libprocess/include/process/metrics/gauge.hpp af06534622b3f00c1d1dc5c577527375318fd79a
3rdparty/libprocess/src/tests/metrics_tests.cpp 75f1f0e35fdb1ec707785d1ebb903af99551bdaa
Diff: https://reviews.apache.org/r/20995/diff/
Testing
-------
make check
Thanks,
Dominic Hamon