> On Feb. 6, 2015, 10:56 a.m., Vinod Kone wrote: > > src/master/master.cpp, line 3601 > > <https://reviews.apache.org/r/27531/diff/7/?file=852243#file852243line3601> > > > > Looks like you missed updating metrics here? > > > > Now that I think about it, it's worthwhile to consolidate metrics > > updates into one place to avoid misses like these in the future. How about > > calling incrementTasksStates() in one place, inside forward(). forward() is > > called by every status update generated/received by the master (exception > > is reconcileTasks() but it should probably use forward() too). > > > > ``` > > void forward( > > const StatusUpdate& update, > > const UPID& acknowledgee, > > Framework* framework) > > { > > .... > > > > // Update terminal state metrics. > > if (protobuf::isTerminalState(update.status.state()) { > > incrementTasksStates(...); > > > > switch (update.status.state()) { > > case TASK_FINISHED: ++metrics->tasks_finished; break; > > case TASK_FAILED: ++metrics->tasks_failed; break; > > case TASK_KILLED: ++metrics->tasks_killed; break; > > case TASK_LOST: ++metrics->tasks_lost; break; > > default: break; > > } > > } > > } > > ``` > > Vinod Kone wrote: > actually, both reconcileTasks() and removeFramework() seem to be > exceptions. hmmm.. > > Dominic Hamon wrote: > i like this. i did a scan of uses of 'forward' originally and thought it > was being called in too many places to be a good candidate for when we want > to update the metrics. filtering on terminal states is the right way to do it > though. > > and yes, reconciliation is subtle. if it does use forward, we may end up > counting duplicates, though the reason will be 'reconciliation' so it should > be ok. > > Dominic Hamon wrote: > removeFramework calls updateTask which updates terminal states.
statusUpdate calls both forward() and updateTask() so that might double count now. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27531/#review71470 ----------------------------------------------------------- On Feb. 6, 2015, 10:42 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27531/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2015, 10:42 a.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 c3c77f840f089c5754c764e7f150a3c1971d311f > src/master/master.cpp 22fece79c6e511a1b61eb674d7f82216e7a25e00 > src/master/metrics.hpp ee8b5bc76336ece9192455b216187482cee29420 > src/master/metrics.cpp 0b3b91e744cdb76cd843edfcf34b12b842fcb06e > src/slave/slave.hpp 9adee17cb94a72f0e1e139b3fd8978a9a1ff6237 > src/slave/slave.cpp 336e8776be656c5e6ad1b0d997f54c307f4559f8 > src/tests/master_authorization_tests.cpp > 6fd0efa444dce5203e2e755d4de0483fcaa277f8 > src/tests/master_slave_reconciliation_tests.cpp > 09742855c9fcd6bd10ab79af582b14fba2e6b609 > src/tests/master_tests.cpp 62ba35b9c3f999c59a95bffb01b8b82cc543a34f > src/tests/mesos.hpp 83a369968ab2403fa341829ac5d11f7243095190 > src/tests/mesos.cpp 21a405366f56c963611324076efe775f85b9d9f7 > src/tests/rate_limiting_tests.cpp 8b55bffff0b7713806fc0bcc63d8a7097a8408e0 > src/tests/slave_tests.cpp 68a6498cd86723e571b262b2495b4b504e744428 > > Diff: https://reviews.apache.org/r/27531/diff/ > > > Testing > ------- > > added metric tests to master tests > make check > > > Thanks, > > Dominic Hamon > >
