> On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/master.hpp > > Lines 941-944 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129569#file2129569line941> > > > > Is this used anywhere?
It is not. Removed! > On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/metrics.cpp > > Lines 158-159 (original), 160-161 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129572#file2129572line160> > > > > Should we add a similar metric for operation status updates? Sounds reasonable. > On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/metrics.cpp > > Lines 174-177 (original), 176-179 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129572#file2129572line176> > > > > Should we add a similar metrics for operation status updates? Sounds equally reasonable. > On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/metrics.cpp > > Lines 386-388 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129572#file2129572line386> > > > > This is weirdly formatted, I think the following follows the style > > guide and is more readable: > > > > ``` > > if (type == Offer::Operation::UNKNOWN || > > type == Offer::Operation::LAUNCH || > > type == Offer::Operation::LAUNCH_GROUP) { > > ``` Both styles are explicitly permitted by the style guide, as long as the same style is used consistently within a given boolean expression. But since you feel this one looks better I will change it. > On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/metrics.cpp > > Lines 637-651 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129572#file2129572line637> > > > > Can these counters be decremented? If not, should we add a CHECK to > > ensure that `delta` is always positive? They can be decremented, and we expect delta to be sometimes negative in this function. I think the only functional difference between `Counter` and `PushGauge` is the existence of `operator=`. > On March 8, 2019, 12:30 a.m., Gastón Kleiman wrote: > > src/master/metrics.cpp > > Lines 684 (patched) > > <https://reviews.apache.org/r/70116/diff/2/?file=2129572#file2129572line684> > > > > Can counters for terminal states be decremented? I guess not? If I'm > > correct, then we could add a `CHECK` here. They can be decremented, but they should not be unless we have a bug in the master code. I'm still a bit hesitant to `CHECK` for this, please see my comment to a similar suggestion by Greg above. - Benno ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70116/#review213541 ----------------------------------------------------------- On March 8, 2019, 11:48 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70116/ > ----------------------------------------------------------- > > (Updated March 8, 2019, 11:48 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu. > > > Bugs: MESOS-8241 > https://issues.apache.org/jira/browse/MESOS-8241 > > > Repository: mesos > > > Description > ------- > > This commit adds additional metrics counting the > number of operations in each state. > > Unit tests are added in the subsequent commit. > > > Diffs > ----- > > src/master/master.cpp dc68fc324de7242737123015fbac19a2129778ce > src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa > src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 > > > Diff: https://reviews.apache.org/r/70116/diff/3/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
