> On May 16, 2018, 10:34 a.m., Greg Mann wrote: > > src/master/master.cpp > > Lines 12113 (patched) > > <https://reviews.apache.org/r/66846/diff/5/?file=2023802#file2023802line12113> > > > > When `task->statuses.size == 0`, I think we probably want to increment > > the terminal task state metric using the contents of `task->state()`.
does `task->statuses.size == 0` case exist? > On May 16, 2018, 10:34 a.m., Greg Mann wrote: > > src/master/master.cpp > > Lines 12115 (patched) > > <https://reviews.apache.org/r/66846/diff/5/?file=2023802#file2023802line12115> > > > > In this case, maybe we still want to increment the metric? We should > > probably figure out which of the two fields (`task->state()` or > > `task->statuses()`) should contain the more recent state and use that to > > increment the metric. > > > > Or, do we expect this case to never happen? I don't think this could happen. user warning instead of CHECK, just to be more safe > On May 16, 2018, 10:34 a.m., Greg Mann wrote: > > src/master/metrics.cpp > > Lines 683 (patched) > > <https://reviews.apache.org/r/66846/diff/5/?file=2023804#file2023804line683> > > > > Is this temporary variable necessary? Here and below. as mentioned in previous patches, it wouldn't compile due to unordered_map++ issue. - Gilbert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66846/#review203253 ----------------------------------------------------------- On May 15, 2018, 5:45 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66846/ > ----------------------------------------------------------- > > (Updated May 15, 2018, 5:45 p.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, > and Vinod Kone. > > > Bugs: MESOS-8912 > https://issues.apache.org/jira/browse/MESOS-8912 > > > Repository: mesos > > > Description > ------- > > Added per framework metrics support for terminal task state. > > > Diffs > ----- > > src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e > src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b > src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 > > > Diff: https://reviews.apache.org/r/66846/diff/5/ > > > Testing > ------- > > > Thanks, > > Gilbert Song > >
