The longer names do seem to help for 'update', sounds good! I also agree
about the general rule you're advocating for.
What are your concrete thoughts for removeSlave? Michael's comment was
inaccurate in that the intention of both is to remove the slave from the
registry. Initially, we only had removeSlave(Slave*), and as I introduced
the recovery code, we unfortunately could not call removeSlave(Slave*)
because we didn't have a Slave* (out of technical debt [1] [2]). Therefore
we added an overload in which we passed a registry::Slave. I can see how
this might be confusing but the documentation seems to be clarify it:
// Remove the slave from the registrar. Called when the slave
// does not re-register in time after a master failover.
Nothing removeSlave(const Registry::Slave& slave);
// Remove the slave from the registrar and from the master's state.
//
// TODO(bmahler): 'reason' is optional until MESOS-2317 is resolved.
void removeSlave(
Slave* slave,
const std::string& message,
Option<process::metrics::Counter> reason = None());
It seems inaccurate to say that these have completely different semantics.
The intention was for these to have the same semantics of removing a slave
from the registry (and all that it entails). I'd sooner try to consolidate
these functions (ideally, a recovered slave could be a Slave*, or we can
just take the SlaveID, etc) than try to come up with a different naming
scheme. Thoughts?
[1] Originally I filed https://issues.apache.org/jira/browse/MESOS-2423 for
the discrepancy.
[2] After Neil and I discussed, we decided to file the more generally
described https://issues.apache.org/jira/browse/MESOS-4048 (I forgot that I
had already filed MESOS-2423 at the time).
On Mon, Jul 4, 2016 at 2:31 AM, Neil Conway <[email protected]> wrote:
> On Sun, Jul 3, 2016 at 9:10 PM, Benjamin Mahler <[email protected]>
> wrote:
> > To clarify, are you ok with the removeSlave example? It seems to fit your
> > criteria.
>
> I think `removeSlave` is poorly named, for similar reasons -- I just
> talked about `update` in my email for brevity.
>
> > Usually with this kind of email we need concrete suggestions for
> > improvement.
>
> My email included the following, which I think is pretty concrete:
>
> ***
> I'd like to propose that we avoid naming functions in this style: if
> two functions do fundamentally different things or should be invoked
> in very different circumstances, we should avoid giving them the same
> name. We should use overloading when two variants of a function do
> basically the same thing but differ slightly in the parameters they
> accept.
> ***
>
> We can certainly debate the specifics of whether/how to rename
> particular functions, but I think the bigger question is whether we
> want to endorse using overloading to differentiate between functions
> that do fundamentally different things.
>
> > With this I don't think it's as bad as you've described in terms of being
> > able to intuit behavior:
> >
> > sorter->update(client, weight);
> > sorter->update(slaveId, newTotal);
> > sorter->update(client, slaveId, oldAllocation, newAllocation);
> >
> > That being said, if there are more helpful function names let's make some
> > suggestions! The obvious alternative here seems to be verbose names that
> > repeat the arguments?
> >
> > sorter->update_client_weight(client, weight);
> > sorter->update_slave_total(slave, total);
> > sorter->update_client_allocation(client, slaveId, oldAllocation,
> > newAllocation);
> >
> > We tend to avoid this pattern as well, because it leads to redundancy.
>
> I would happily accept a little more redundancy for these examples,
> because I think it improves the clarity of the code. We tend to favor
> clarity and redundancy over brevity in several other situations (e.g.,
> variable names must be entire words, using `load` and `store` for
> atomics rather than operator overloading).
>
> For the specific examples above, I think the longer names (e.g.,
> `update_client_weight`) are an improvement. There's also a compromise
> (adding back the fourth variant of `update` which is a private
> function):
>
> sorter->updateWeight(client, weight);
> sorter->updateTotal(slaveId, total);
> sorter->updateAllocation(client, slaveId, oldAllocation, newAllocation);
> sorter->updateShare(client);
>
> Neil
>
