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/03/20 23:35:01 UTC
Review Request 19504: Ported master stats.json to use new metrics library.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review38293
-----------------------------------------------------------
Bad patch!
Reviews applied: [18718, 19504]
Failed command: make -j3 check GTEST_FILTER='' >/dev/null
Error:
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
In file included from src/tests/metrics_tests.cpp:5:0:
./include/process/metrics/counter.hpp: In member function 'virtual double process::metrics::Counter::get() const':
./include/process/metrics/counter.hpp:21:48: error: invalid conversion from 'const volatile void*' to 'volatile void*' [-fpermissive]
make[5]: *** [tests-metrics_tests.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [check-am] Error 2
make[3]: *** [check-recursive] Error 1
make[2]: *** [check-recursive] Error 1
make[1]: *** [check] Error 2
make: *** [check-recursive] Error 1
- Mesos ReviewBot
On March 20, 2014, 10:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 10:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41609
-----------------------------------------------------------
Are you planning on rebasing this and the other metric integration patches?
- Ben Mahler
On April 10, 2014, 11:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 11:14 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/master.cpp 3c3c989543167afb7d368a19a16457ed00e6be0c
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review40111
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19504]
All tests passed.
- Mesos ReviewBot
On April 10, 2014, 11:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated April 10, 2014, 11:14 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/master.cpp 3c3c989543167afb7d368a19a16457ed00e6be0c
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 9, 2014, 5:35 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 566-569
> > <https://reviews.apache.org/r/19504/diff/11/?file=577350#file577350line566>
> >
> > 'slaves.deactivated' is actually the _removed_ slaves!
> >
> > We'll want 'active_slaves' to count those slaves in 'slaves.activated' that are connected, and 'inactive_slaves' to count those slaves in 'slaves.activated' that are disconnected (no offers being sent for them).
in Slave::addOffer we don't check disconnected - should we?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review42595
-----------------------------------------------------------
On May 12, 2014, 10:21 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 10:21 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132 and MESOS-1332
> https://issues.apache.org/jira/browse/MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1332
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
> src/master/master.cpp d851a7291acce950ea9391ddfb8813a432aeda34
> src/tests/master_tests.cpp 30ea7ffad4eb0ad011942d91cbf61284b031a80e
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review42595
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment76443>
I would propose the following order (keeps the message counters together and tries to group them in an intuitive way) and let's use this same order in the initializer list, constructor body, and destructor body:
process::metrics::Gauge uptime_secs;
process::metrics::Gauge elected;
process::metrics::Gauge active_slaves;
process::metrics::Gauge inactive_slaves;
process::metrics::Gauge active_frameworks;
process::metrics::Gauge inactive_frameworks;
process::metrics::Gauge outstanding_offers;
// Message counters.
// TODO(bmahler): Add counters for other messages: kill task,
// status update, etc.
process::metrics::Counter dropped_messages;
process::metrics::Counter framework_registration_messages;
process::metrics::Counter framework_reregistration_messages;
process::metrics::Counter slave_registration_messages;
process::metrics::Counter slave_reregistration_messages;
process::metrics::Counter valid_framework_messages;
process::metrics::Counter invalid_framework_messages;
process::metrics::Counter valid_status_updates;
process::metrics::Counter invalid_status_updates;
// Recovery counters.
process::metrics::Counter recovery_slave_removals;
// Process metrics.
process::metrics::Gauge event_queue_size;
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment76428>
Hm, these are a bit confusing because they sound like they are referring to _all_ framework messages but they're only referring to framework -> executor messages, let's update these names to:
valid_framework_to_executor_messages
invalid_framework_to_executor_messages
Let's also follow up on the slave metrics to make a similar naming change (these were missed in my list in MESOS-1332 because they weren't in the JSON).
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment76444>
'slaves.deactivated' is actually the _removed_ slaves!
We'll want 'active_slaves' to count those slaves in 'slaves.activated' that are connected, and 'inactive_slaves' to count those slaves in 'slaves.activated' that are disconnected (no offers being sent for them).
- Ben Mahler
On May 9, 2014, 5:37 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 9, 2014, 5:37 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132 and MESOS-1332
> https://issues.apache.org/jira/browse/MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1332
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
> src/master/master.cpp d851a7291acce950ea9391ddfb8813a432aeda34
> src/tests/master_tests.cpp 326392ed03f3cb1321ec21af57a5b0253cf29ef9
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 14, 2014, 12:13 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3670-3696
> > <https://reviews.apache.org/r/19504/diff/17/?file=581329#file581329line3670>
> >
> > Wouldn't the following be simpler and easier to understand for those reading these functions?
> >
> > double Master::_active_slaves()
> > {
> > double count = 0.0;
> > foreachvalue (Slave* slave, slaves.activated) {
> > if (!slave->disconnected) {
> > count++;
> > }
> > }
> > return count;
> > }
> >
> >
> > double Master::_inactive_slaves()
> > {
> > double count = 0.0;
> > foreachvalue (Slave* slave, slaves.activated) {
> > if (slave->disconnected) {
> > count++;
> > }
> > }
> > return count;
> > }
> >
> > It seems odd that the forward declaration of 'struct Slave' was not sufficient to place these in the header, since Slave was also defined in the header further down, ah well.
Thanks!
I have always preferred to use the standard algorithms wherever possible as they may have optimal implementations for some container types (not in this case, probably). I find them quite readable but I do like the explicit loop too.
The forward declaration is just that; it tells the compiler "There's a symbol named Slave". To compile these method definitions the compiler would need to know the symbol named Slave has a field named 'disconnected' that is convertible to bool. For that, it needs the full definition to be available.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review43001
-----------------------------------------------------------
On May 13, 2014, 9:53 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 9:53 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132 and MESOS-1332
> https://issues.apache.org/jira/browse/MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1332
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 4f9ae36c822a16ea3baadf6b9fa3616d030d19f2
> src/master/master.cpp d5453673a839326d00a3d45940bd4562c526cff2
> src/tests/master_tests.cpp 7aa678afc94869c8243485bd0604532dec43a1e2
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review43001
-----------------------------------------------------------
Ship it!
Thanks!! Getting this committed with the change described below to simplify the Gauge functions.
src/master/master.cpp
<https://reviews.apache.org/r/19504/#comment76986>
Wouldn't the following be simpler and easier to understand for those reading these functions?
double Master::_active_slaves()
{
double count = 0.0;
foreachvalue (Slave* slave, slaves.activated) {
if (!slave->disconnected) {
count++;
}
}
return count;
}
double Master::_inactive_slaves()
{
double count = 0.0;
foreachvalue (Slave* slave, slaves.activated) {
if (slave->disconnected) {
count++;
}
}
return count;
}
It seems odd that the forward declaration of 'struct Slave' was not sufficient to place these in the header, since Slave was also defined in the header further down, ah well.
- Ben Mahler
On May 14, 2014, 4:53 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 4:53 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132 and MESOS-1332
> https://issues.apache.org/jira/browse/MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1332
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 4f9ae36c822a16ea3baadf6b9fa3616d030d19f2
> src/master/master.cpp d5453673a839326d00a3d45940bd4562c526cff2
> src/tests/master_tests.cpp 7aa678afc94869c8243485bd0604532dec43a1e2
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 13, 2014, 9:53 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased to master
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 4f9ae36c822a16ea3baadf6b9fa3616d030d19f2
src/master/master.cpp d5453673a839326d00a3d45940bd4562c526cff2
src/tests/master_tests.cpp 7aa678afc94869c8243485bd0604532dec43a1e2
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 13, 2014, 10:52 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 923552e3c7f9942d6be7a0446b44e40f53c11433
src/master/master.cpp 569995c2eaf758f5171c4e0d082d4e519a143025
src/tests/master_tests.cpp 639e7607a6ab8a6e806177d2c6257e79f6820d6f
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 12, 2014, 4:01 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased again
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 923552e3c7f9942d6be7a0446b44e40f53c11433
src/master/master.cpp 569995c2eaf758f5171c4e0d082d4e519a143025
src/tests/master_tests.cpp 639e7607a6ab8a6e806177d2c6257e79f6820d6f
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 12, 2014, 3:55 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 923552e3c7f9942d6be7a0446b44e40f53c11433
src/master/master.cpp 569995c2eaf758f5171c4e0d082d4e519a143025
src/tests/master_tests.cpp 639e7607a6ab8a6e806177d2c6257e79f6820d6f
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 12, 2014, 10:21 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
src/master/master.cpp d851a7291acce950ea9391ddfb8813a432aeda34
src/tests/master_tests.cpp 30ea7ffad4eb0ad011942d91cbf61284b031a80e
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 12, 2014, 10:20 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
updated definition of (in)active_slaves and reordered metrics.
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
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
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
src/master/master.cpp d851a7291acce950ea9391ddfb8813a432aeda34
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/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 9, 2014, 10:37 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
updated metric names as per MESOS-1332.
Bugs: MESOS-1132 and MESOS-1332
https://issues.apache.org/jira/browse/MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1332
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
src/master/master.cpp d851a7291acce950ea9391ddfb8813a432aeda34
src/tests/master_tests.cpp 326392ed03f3cb1321ec21af57a5b0253cf29ef9
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 8, 2014, 12:01 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
added to test
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
src/tests/master_tests.cpp 939a08d0ed25293d103728864c2b5e79f04a1b42
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 8, 2014, 11:38 a.m., Ben Mahler wrote:
> > src/master/http.cpp, line 380
> > <https://reviews.apache.org/r/19504/diff/9/?file=574761#file574761line380>
> >
> > Can you link up the review for this TODO so that it depends on this one?
that review depends on the defer being removed from Gauge (review 20995) so i can't have it depend on this too.
> On May 8, 2014, 11:38 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3660-3670
> > <https://reviews.apache.org/r/19504/diff/9/?file=574763#file574763line3660>
> >
> > Can this go up with the other gauge methods?
it requires Framework to be a complete type so yes, if we move the definition of Framework above Master in the header. This felt more disruptive than just having this in the source file.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review42521
-----------------------------------------------------------
On May 8, 2014, 12:01 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 12:01 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
> src/tests/master_tests.cpp 939a08d0ed25293d103728864c2b5e79f04a1b42
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review42521
-----------------------------------------------------------
Let's add to the MasterTest.MetricsInStatsEndpoint to make sure we've converted everything!
A TODO for master/metrics.cpp might be nice as well.
src/master/http.cpp
<https://reviews.apache.org/r/19504/#comment76312>
Can you link up the review for this TODO so that it depends on this one?
src/master/master.cpp
<https://reviews.apache.org/r/19504/#comment76311>
Can this go up with the other gauge methods?
- Ben Mahler
On May 5, 2014, 5:42 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 5:42 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
> src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 5, 2014, 10:42 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 0a350b0b402edb3ca648c91c920043f66c08fe0e
src/master/master.cpp c79fdafde6b08d74e6fea71afa79aded41b3a9ab
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review42120
-----------------------------------------------------------
Bad patch!
Reviews applied: [19504]
Failed command: git apply --index 19504.patch
Error:
error: patch failed: src/master/master.hpp:36
error: src/master/master.hpp: patch does not apply
error: patch failed: src/master/master.cpp:221
error: src/master/master.cpp: patch does not apply
- Mesos ReviewBot
On May 2, 2014, 1:44 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 1:44 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated May 1, 2014, 6:44 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
benm's review
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > Could we have a small integration test that starts a Master and ensures the right names are present in /stats.json and /metrics/snapshot?
> >
> > I think the comments I made here would generalize to the slave patch as well, could you take them into account there as well?
I can add that test - given that we have tests that already check if a metric shows up in the endpoint when it's added to the metrics system I'm not sure what it would add to our test coverage. I'm also concerned that it might be fragile as it would need to be updated as people start adding and removing metrics. I may be missing something: what do you expect it to show us over what we already test?
Yes - i'll apply anything relevant to the Slave patch.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/http.cpp, line 380
> > <https://reviews.apache.org/r/19504/diff/7/?file=571713#file571713line380>
> >
> > Any plan to follow up for this one later? (Would be nice to have a complete deprecation for all old statistics in 0.19.0, without these we aren't offering the data in both endpoints).
Yes, absolutely. I wanted the metrics library to settle and these patches to land so I had a better idea of what I'd need to do to port them. I already had the slave and master patches out and wanted to avoid making similar changes to each.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 522-534
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line522>
> >
> > Could we rename these to match the exposed metric's names? (Similar to how the one's below are named, e.g. "dropped_messages" over "droppedMessages").
> >
> > s/isElected/elected/ as well?
> >
> > Ditto for the gauge methods themselves (akin to how we named RegistrarProcess::_queued_operations).
Absolutely. I'm catching up with the rules we've established so far :)
Thanks for catching this.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > Can we just keep this in the header as before so it's all in one place for now?
> >
> > We could think of ways to clean things up later :)
at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3680-3703
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3680>
> >
> > How about just placing each argument on a newline so that this is a bit easier on the eyes?
> >
> > E.g.
> >
> > invalid_framework_messages(
> > "master/invalid_framework_messages"),
> > uptime(
> > "master/uptime",
> > defer(master, &Master::_uptime)),
> > elected(
> > "master/elected",
> > defer(master, &Master::_elected)),
> > total_schedulers(
> > "master/total_schedulers",
> > defer(master, &Master::_totalSchedulers)),
> > ...
> > dropped_messages(
> > "master/dropped_messages"),
> >
done - i'm not sure it's much better to read, but i have tried a few ways and nothing really works :/
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 377-392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line377>
> >
> > Should all of these be 'const'?
yes - but Gauge doesn't support that. I considered having Gauge require the functions passed in to be <Future<double> void() const> but felt it would be too restrictive for users. There might be cases that users want to, say, acquire a lock before accessing data. That would then require mutable and that way madness lies!
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 1, 2014, 6:44 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 6:44 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
>
> Dominic Hamon wrote:
> yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
>
> until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
>
> Ben Mahler wrote:
> That seems odd, even though 'Framework' is defined in the header below 'Master', it is forward declared at the top, which should solve this issue? What was the compiler error?
the implementation of _activeTasks (_active_tasks) requires Framework to be complete as it accesses members. We could just move Framework to above Master in the header.
> On May 1, 2014, 4:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > Can we just keep this in the header as before so it's all in one place for now?
> >
> > We could think of ways to clean things up later :)
>
> Dominic Hamon wrote:
> at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
>
> Ben Mahler wrote:
> I would think less about the "user" of Master, because we don't "use" Master as a library. Rather I would think about the "reader" of the code, who's trying to understand how the Master works, what kinds of things are exposed. That being said, you're right that this is quite a bit of code bloat and could go into the source file, my only point would be that it's nice for the "reader" of the code to be able to see all of the metrics names in the header.
>
> Up to you!
Wrote this in email but it didn't show up here so, for posterity:
?That's a really good point regarding the metrics names that end up being published in the endpoint. As long as we're consistent about naming the metric variables the same as their eventual endpoint names, I'm comfortable that readers of the code won't be confused, and having the methods in the source make it simpler to see which metrics are being exposed. We just need to be conscious that we've established this precedent for naming :)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 1, 2014, 6:44 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 6:44 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
On Fri, May 2, 2014 at 10:46 AM, Ben Mahler <be...@gmail.com>wrote:
>
>
> > On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > > src/master/master.hpp, line 392
> > > <
> https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> > >
> > > Is it possible to move this one up into the header?
> >
> > Dominic Hamon wrote:
> > yes, if i move struct Framework {...}; above Master (or into its own
> header (hint hint)).
> >
> > until then, it can be in the header but not within Master's class
> definition. Given no other methods are in the header but outside the class
> definition, I'll keep it in the source file for consistency.
>
> That seems odd, even though 'Framework' is defined in the header below
> 'Master', it is forward declared at the top, which should solve this issue?
> What was the compiler error?
>
The implementation of the method needs Framework to be a complete type as
it is accessing members. If you'd rather I just move Framework above Master
and remove the forward declaration, I can.
>
>
> > On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > > src/master/master.cpp, line 3679
> > > <
> https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> > >
> > > Can we just keep this in the header as before so it's all in one
> place for now?
> > >
> > > We could think of ways to clean things up later :)
> >
> > Dominic Hamon wrote:
> > at nearly 60 lines of (frankly) ugly looking repetitive code, i
> think it clutters up an already complicated class header too much. I'd much
> rather keep it out - the details of what it's doing aren't particularly
> challenging to guess so having it in the header doesn't aid the user of
> Master at all.
>
> I would think less about the "user" of Master, because we don't "use"
> Master as a library. Rather I would think about the "reader" of the code,
> who's trying to understand how the Master works, what kinds of things are
> exposed. That being said, you're right that this is quite a bit of code
> bloat and could go into the source file, my only point would be that it's
> nice for the "reader" of the code to be able to see all of the metrics
> names in the header.
>
>
That's a really good point regarding the metrics names that end up being
published in the endpoint. As long as we're consistent about naming the
metric variables the same as their eventual endpoint names, I'm comfortable
that readers of the code won't be confused, and having the methods in the
source make it simpler to see which metrics are being exposed. We just need
to be conscious that we've established this precedent for naming :)
>
>
>
> Up to you!
>
>
> - Ben
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/#review41973
> -----------------------------------------------------------
>
>
> On May 2, 2014, 1:44 a.m., Dominic Hamon wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/19504/
> > -----------------------------------------------------------
> >
> > (Updated May 2, 2014, 1:44 a.m.)
> >
> >
> > Review request for mesos and Ben Mahler.
> >
> >
> > Bugs: MESOS-1132
> > https://issues.apache.org/jira/browse/MESOS-1132
> >
> >
> > Repository: mesos-git
> >
> >
> > Description
> > -------
> >
> > see summary
> >
> >
> > Diffs
> > -----
> >
> > src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> > src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> > src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
> >
> > Diff: https://reviews.apache.org/r/19504/diff/
> >
> >
> > Testing
> > -------
> >
> > ran locally and checked /metrics/snapshot vs master/stats.json
> >
> > make check
> >
> >
> > Thanks,
> >
> > Dominic Hamon
> >
> >
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
> On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 392
> > <https://reviews.apache.org/r/19504/diff/7/?file=571714#file571714line392>
> >
> > Is it possible to move this one up into the header?
>
> Dominic Hamon wrote:
> yes, if i move struct Framework {...}; above Master (or into its own header (hint hint)).
>
> until then, it can be in the header but not within Master's class definition. Given no other methods are in the header but outside the class definition, I'll keep it in the source file for consistency.
That seems odd, even though 'Framework' is defined in the header below 'Master', it is forward declared at the top, which should solve this issue? What was the compiler error?
> On May 1, 2014, 11:23 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 3679
> > <https://reviews.apache.org/r/19504/diff/7/?file=571715#file571715line3679>
> >
> > Can we just keep this in the header as before so it's all in one place for now?
> >
> > We could think of ways to clean things up later :)
>
> Dominic Hamon wrote:
> at nearly 60 lines of (frankly) ugly looking repetitive code, i think it clutters up an already complicated class header too much. I'd much rather keep it out - the details of what it's doing aren't particularly challenging to guess so having it in the header doesn't aid the user of Master at all.
I would think less about the "user" of Master, because we don't "use" Master as a library. Rather I would think about the "reader" of the code, who's trying to understand how the Master works, what kinds of things are exposed. That being said, you're right that this is quite a bit of code bloat and could go into the source file, my only point would be that it's nice for the "reader" of the code to be able to see all of the metrics names in the header.
Up to you!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
On May 2, 2014, 1:44 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 1:44 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review41973
-----------------------------------------------------------
Could we have a small integration test that starts a Master and ensures the right names are present in /stats.json and /metrics/snapshot?
I think the comments I made here would generalize to the slave patch as well, could you take them into account there as well?
src/master/http.cpp
<https://reviews.apache.org/r/19504/#comment75653>
Any plan to follow up for this one later? (Would be nice to have a complete deprecation for all old statistics in 0.19.0, without these we aren't offering the data in both endpoints).
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment75658>
Should all of these be 'const'?
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment75655>
Is it possible to move this one up into the header?
src/master/master.hpp
<https://reviews.apache.org/r/19504/#comment75654>
Could we rename these to match the exposed metric's names? (Similar to how the one's below are named, e.g. "dropped_messages" over "droppedMessages").
s/isElected/elected/ as well?
Ditto for the gauge methods themselves (akin to how we named RegistrarProcess::_queued_operations).
src/master/master.cpp
<https://reviews.apache.org/r/19504/#comment75657>
Can we just keep this in the header as before so it's all in one place for now?
We could think of ways to clean things up later :)
src/master/master.cpp
<https://reviews.apache.org/r/19504/#comment75656>
How about just placing each argument on a newline so that this is a bit easier on the eyes?
E.g.
invalid_framework_messages(
"master/invalid_framework_messages"),
uptime(
"master/uptime",
defer(master, &Master::_uptime)),
elected(
"master/elected",
defer(master, &Master::_elected)),
total_schedulers(
"master/total_schedulers",
defer(master, &Master::_totalSchedulers)),
...
dropped_messages(
"master/dropped_messages"),
- Ben Mahler
On April 30, 2014, 6:11 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated April 30, 2014, 6:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
> src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
> src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics/snapshot vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated April 30, 2014, 11:11 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp f54c841d003d7a4d2eda5e93405afa358707f4e0
src/master/master.hpp 333e37ccd5746c5026740cfcb816499cea61a545
src/master/master.cpp f205dca43f10697862e3fd3f435f1127a9d0aecb
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats to use new metrics library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated April 10, 2014, 4:14 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased to master.
Summary (updated)
-----------------
Ported master stats to use new metrics library.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
src/master/master.cpp 3c3c989543167afb7d368a19a16457ed00e6be0c
Diff: https://reviews.apache.org/r/19504/diff/
Testing (updated)
-------
ran locally and checked /metrics/snapshot vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated March 25, 2014, 3:26 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased and added back old stats for comparison.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp a8ed5ec55766b7ecf3ed1368916da8b4b3e5bbe8
src/master/master.cpp 544144de3d955000cce58371ddc57d53e0941aae
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated March 25, 2014, 11:19 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp a8ed5ec55766b7ecf3ed1368916da8b4b3e5bbe8
src/master/master.cpp 544144de3d955000cce58371ddc57d53e0941aae
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated March 24, 2014, 12:16 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased to 18718 changes to support deferred objects in Gauge.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/#review38309
-----------------------------------------------------------
Same high-level comments from https://reviews.apache.org/r/19499 apply to this review too.
- Benjamin Hindman
On March 20, 2014, 10:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19504/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 10:52 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1132
> https://issues.apache.org/jira/browse/MESOS-1132
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
>
> Diff: https://reviews.apache.org/r/19504/diff/
>
>
> Testing
> -------
>
> ran locally and checked /metrics vs master/stats.json
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated March 20, 2014, 3:52 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon
Re: Review Request 19504: Ported master stats.json to use new metrics
library.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19504/
-----------------------------------------------------------
(Updated March 20, 2014, 3:51 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1132
https://issues.apache.org/jira/browse/MESOS-1132
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
src/master/http.cpp 72d8e91013cd6a9c52a6d1ffdf517a52c4567bb7
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
Diff: https://reviews.apache.org/r/19504/diff/
Testing
-------
ran locally and checked /metrics vs master/stats.json
make check
Thanks,
Dominic Hamon