> On Feb. 4, 2015, 8:41 p.m., Vinod Kone wrote: > > src/tests/master_authorization_tests.cpp, lines 365-373 > > <https://reviews.apache.org/r/27531/diff/4/?file=768126#file768126line365> > > > > It is a bit weird to see a test for these metrics in authorization > > tests. Any particular reason or did you randomly pick this file? I would > > recommend testing these in master_tests.cpp (and in slave_tests.cpp once > > you implement the slave metrics) since that already has metrics tests. > > Dominic Hamon wrote: > when implementing metrics, there was discussion around having metric > tests or inserting metric tests into existing tests. The conclusion there was > that introducing metric tests would require duplication of the logic from > many other existing tests to trigger different updates, and better metrics > coverage could be tested by inserting metric tests into existing tests. > > this is a prime example where expectations of task state, sources and > reasons are well defined by the existing tests.
i just looked at master_tests, slave_tests and fault_tolerance_tests and none of them seem to test disconnected slave, though there was a test for removed slave. so testing the metrics here is fine. will drop the issue. > On Feb. 4, 2015, 8:41 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 4704 > > <https://reviews.apache.org/r/27531/diff/4/?file=768121#file768121line4704> > > > > Not sure why we missed updating "metrics" for non-terminal states when > > we introduced "metrics". This is a bug. I'll prep a fix for this. > > Dominic Hamon wrote: > there was a discussion about this in the reviews but i don't remember the > outcome. i think this is because 'tasks_states' is a counter and using counters (instead of gauges) for non-terminal states is weird. will drop the issue. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27531/#review71023 ----------------------------------------------------------- On Feb. 6, 2015, 5:59 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27531/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2015, 5:59 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-1830 > https://issues.apache.org/jira/browse/MESOS-1830 > > > Repository: mesos > > > Description > ------- > > Update metrics in Master to match the source and reason split for task > statuses. > > > Diffs > ----- > > src/master/master.hpp dcfd38ae2fa9e1bd0b477e9719724dba37114d30 > src/master/master.cpp 234bbecc4205036d790b026abd59100eb188f913 > src/master/metrics.hpp 5e18f883fe82ae0b7ce1bd90419e271409867289 > src/master/metrics.cpp 5fe71e3545b48b26a7bc50252d5707cad453bffc > src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6 > src/slave/slave.cpp a8b262174ab5c9a524db8318d3d1438cd75a702b > src/tests/master_authorization_tests.cpp > 20adaa954a0ddfe3459d8a3f9696b5ca9ae07f24 > src/tests/master_slave_reconciliation_tests.cpp > 04806ede4e727fa4de1464017a06797d69b54e29 > src/tests/master_tests.cpp 3cb7660b372c9846ae8c0aefbe559095f47e7794 > src/tests/mesos.hpp 17c2d8f0cb6326b08fc506143e823ee2c3a32e09 > src/tests/mesos.cpp 5ed4df530cf1bf11eec3b29542641822e0f702b2 > src/tests/rate_limiting_tests.cpp 7f5ca251fef5b1852bb273505f5eaed750578b9f > src/tests/slave_tests.cpp 956ce64fcbee116a37eaa30b14edf580e7cff888 > > Diff: https://reviews.apache.org/r/27531/diff/ > > > Testing > ------- > > added metric tests to master tests > make check > > > Thanks, > > Dominic Hamon > >
