To clarify, are you ok with the removeSlave example? It seems to fit your
criteria.
Usually with this kind of email we need concrete suggestions for
improvement. First though I wanted to improve some issues around naming and
the descriptions you've used in the update example to avoid conflating
things further (e.g. there are only 3 signatures, the sorter doesn't know
about "roles", the descriptions are not precisely matching the behavior for
(2) and (4), we can improve argument naming). So we have:
void update(const std::string& client, double newWeight);
void update(const SlaveID& slaveId, const Resources& newTotal);
void update(
const std::string& client,
const SlaveID& slaveId,
const Resources& oldAllocation,
const Resources& newAllocation);
(1) Update a client's weight.
(2) Update the total resources for a slave.
(3) Update a client's allocation on a slave.
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. For
example, we would generally prefer:
map.count(key)
map.contains(key)
and not:
map.countKey(key)
map.containsKey(key)
Another thought is in the direction of exposing Sorter::Client /
Sorter::Slave to make this more straightforward. Something like:
sorter->clients.at(client).set_weight(weight);
sorter->slaves.at(slaveId).set_total(resources);
sorter->slaves.at(slaveId).update_allocation(client, old, new);
I don't know if you had specific suggestions in mind but would love to hear
them!
On Fri, Jul 1, 2016 at 10:44 AM, Neil Conway <[email protected]> wrote:
> Consider the following function signatures from master.cpp:
>
> Nothing Master::removeSlave(const Registry::Slave& slave);
>
> void Master::removeSlave(Slave* slave, const string& message,
> Option<Counter> reason);
>
> or these from sorter/drf/sorter.hpp:
>
> void update(const SlaveID& slaveId, const Resources& resources);
>
> void update(
> const std::string& name,
> const SlaveID& slaveId,
> const Resources& oldAllocation,
> const Resources& newAllocation);
>
> void update(const std::string& name);
>
> void update(const std::string& name, double weight);
>
> These function names use overloading but the different variants have
> *completely* different semantics. For example, the variants of
> update() do the following:
>
> (1) Set the weight associated with a role.
> (2) Change the total pool of resources in the sorter.
> (3) Update the fair-share associated with a sorter client.
> (4) Change both the total and allocated resources in the sorter.
>
> (For fun, the descriptions and function names are in different orders. :) )
>
> 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.
>
> Comments welcome.
>
> Neil
>
