> 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
> 
>

Reply via email to