----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69719/#review211971 -----------------------------------------------------------
src/resource_provider/manager.cpp Lines 823-832 (original), 827-836 (patched) <https://reviews.apache.org/r/69719/#comment297547> 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. src/resource_provider/manager.cpp Lines 1004-1005 (patched) <https://reviews.apache.org/r/69719/#comment297548> 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") ``` src/tests/resource_provider_manager_tests.cpp Lines 1463-1468 (patched) <https://reviews.apache.org/r/69719/#comment297550> 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 ;) src/tests/resource_provider_manager_tests.cpp Lines 1470 (patched) <https://reviews.apache.org/r/69719/#comment297551> Maybe move all assertions related to a specific metric together? E.g., ``` ASSERT_TRUE(snapshot.values.count(".../subscribed")); EXPECT_EQ(0, snapshot.values.at(".../subscribed")); ASSERT_TRUE(snapshot.values.count(".../subscriptions")); EXPECT_EQ(0, snapshot.values.at(".../subscriptions")); ... ``` src/tests/resource_provider_manager_tests.cpp Lines 1507-1509 (patched) <https://reviews.apache.org/r/69719/#comment297553> Shouldn't we test the existence of the metric keys, since this is a newly generated snapshot? - Chun-Hung Hsiao On Jan. 11, 2019, 10:02 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69719/ > ----------------------------------------------------------- > > (Updated Jan. 11, 2019, 10:02 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 > 20cfb340bf634354865d79c92ee70cddca08f28a > > > Diff: https://reviews.apache.org/r/69719/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
