> On April 17, 2015, 12:38 p.m., Alexander Rojas wrote: > > src/master/master.hpp, line 370 > > <https://reviews.apache.org/r/33154/diff/1/?file=926686#file926686line370> > > > > I find this quite counter intuitive for two reasons: > > 1. If I just check this header, I would expect reason to behave a > > little like an enum, telling me why the slave is being removed. What i > > didn't expect, is that it is a counter to the number of slaves being > > removed by `reason`. > > 2. The parameter is a const ref, last thing I expected is that it would > > change later on. > > > > Can you add a comment explaining the semantics of reason, and if > > possible, getting rid of the const ref?
> 1. If I just check this header, I would expect reason to behave a little like > an enum, telling me why the slave is being removed. What i didn't expect, is > that it is a counter to the number of slaves being removed by reason. Hm.. I'm not sure how one would interpret a '`metrics::Counter reason`' to behave like an enum, since the type itself indicates that this is a reason '`metrics::Counter`' which has a pretty clear pattern of use IMHO. > 2. The parameter is a const ref, last thing I expected is that it would > change later on. Fair point, metrics are shared objects (as many things coming from libprocess are) so that's why this works. However, currently `Option<T>::get` only exposes a const reference as of today, so removing the const ref of Option still requires a copy later on to remove the const ref from `Option<T>::get`. Having a non-const ref version of Option::get has come up before, but I never filed a ticket, here you go: https://issues.apache.org/jira/browse/MESOS-2716 - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/#review80444 ----------------------------------------------------------- On April 14, 2015, 1:46 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33154/ > ----------------------------------------------------------- > > (Updated April 14, 2015, 1:46 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-2485 > https://issues.apache.org/jira/browse/MESOS-2485 > > > Repository: mesos > > > Description > ------- > > See [MESOS-2485](https://issues.apache.org/jira/browse/MESOS-2485). > > > Diffs > ----- > > include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 > src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 > src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 > src/master/metrics.hpp 52a83289cfe7e6b6fd8d5bff0774ebe5ce51d0ed > src/master/metrics.cpp 14486bf7130250f561c9fb7a43c95f3fc1e76a4b > > Diff: https://reviews.apache.org/r/33154/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
