----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27531/#review71313 -----------------------------------------------------------
src/master/master.cpp <https://reviews.apache.org/r/27531/#comment117001> Did you miss this? I see lot of other places in master.cpp where createStatusUpdate was called but metrics were not updated!? Can you make a pass to make sure you catch all of these? Also, thinking a bit more, 'reason' only makes sense for terminal statuses but the hashmap of hashmap of hashmap makes it look like all states are possible, even though your code never touches non-terminal states. You should comment that in metrics.hpp. src/master/master.cpp <https://reviews.apache.org/r/27531/#comment116978> indentation? src/master/master.cpp <https://reviews.apache.org/r/27531/#comment116979> indentation? src/master/metrics.hpp <https://reviews.apache.org/r/27531/#comment116980> s/incrementTasksSourceReason/incrementTasksStates/ to match the variable name. src/master/metrics.cpp <https://reviews.apache.org/r/27531/#comment116998> I would put "strings::lower(TaskState_Name(state)) + "/"" on next line for readability. - Vinod Kone On Feb. 5, 2015, 6:07 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27531/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2015, 6:07 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 > >
