> On Jan. 15, 2019, 12:55 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/manager.cpp > > Lines 823-832 (original), 827-836 (patched) > > <https://reviews.apache.org/r/69719/diff/1/?file=2119190#file2119190line827> > > > > How about moving this snippt to the end of this function, so the agent > > would receive `SUBSCRIBE` and then `DISCONNECT` no matter if the RP has a > > connection problem after it subscribes? This would also make the metric > > reveal the connection problem.
This is supposed to be a counter for successful subscriptions, see below. > On Jan. 15, 2019, 12:55 a.m., Chun-Hung Hsiao wrote: > > src/resource_provider/manager.cpp > > Lines 1004-1005 (patched) > > <https://reviews.apache.org/r/69719/diff/1/?file=2119190#file2119190line1004> > > > > How about the following, to make it consistent with, e.g., > > `master/messages_register_framework`, and also make it less confusing w/ > > `resource_provider_manager/subscribed`? > > ``` > > messages_subscribe("resource_provider_manager/messages_subscribe"), > > messages_disconnect("resource_provider_manager/messages_disconnect") > > ``` These are not really counters for messages as disconnections have no relation to messages the RP manager receives, and the subscriptions counter is supposed to count successfull connections (not just all subscription attempts (this allows correlating subscription and disconnection counters). What do you think about naming them e.g., `resource_provider_manager/events/[subscribe|disconnect]`? We could introduce a separate counter for subscription messages later. > On Jan. 15, 2019, 12:55 a.m., Chun-Hung Hsiao wrote: > > src/tests/resource_provider_manager_tests.cpp > > Lines 1463-1468 (patched) > > <https://reviews.apache.org/r/69719/diff/1/?file=2119191#file2119191line1463> > > > > We usually do the following instead: > > ``` > > ASSERT_NE(0u, snapshot.values.count(...)); > > ``` > > Or > > ``` > > ASSERT_EQ(1u, snapshot.values.count(...)); > > ``` > > > > But I have to admit that `ASSERT_TRUE` is more readable. So it's up to > > you to decide if you want to keep the consistency or not ;) Changed to `ASSERT_EQ`. While I also like above form with `ASSERT_TRUE` Mesos style in general seems to be to avoid implicit conversions to `bool`. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69719/#review211971 ----------------------------------------------------------- On Jan. 15, 2019, 10:27 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69719/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2019, 10:27 a.m.) > > > Review request for mesos, Chun-Hung Hsiao and James DeFelice. > > > Bugs: MESOS-9223 > https://issues.apache.org/jira/browse/MESOS-9223 > > > Repository: mesos > > > Description > ------- > > This patch adds monotonically increasing counters for subscriptions and > disconnections of resource providers with the resource provider manager > (`resource_provider_manager/subscriptions` and > `resource_provider_manager/disconnections`, respectively). While the > existing gauge `resource_provider_manager/subscribed` exposed the > current state, these added counters can be used to monitor resource > provider state for unexpected events. > > > Diffs > ----- > > src/resource_provider/manager.cpp 65852c629393f32fd582bfcff86d7ce14e5386ac > src/tests/resource_provider_manager_tests.cpp > 455ce7d2c71f2815430b69a5475b2ccc343cd9af > > > Diff: https://reviews.apache.org/r/69719/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
