Neil, thanks for bringing this up! Agree with you in principle and
sentiment.
I gave it some thought myself since BenM mentioned providing concrete
suggestions.
Ben, I think Neil brought up `removeSlave` as an example that he's not ok
with. (Neil, correct me if I'm wrong)
As far as I understand it, the issue with the 2 overloads of
`removeSlave` that he mentioned is that one
removes the agent from the registry, whereas the other one removes it from
the in-memory map.
While these are conceptually similar (they do remove the slave in some
way), they do in fact do very different things.
I think an example that Neil is ok with is something like `std::max`.
Specifically these overloads (not sure about the ones with comparators):
* `const T& max(const T&lhs, const T&rhs);`
* `T max(initializer_list<T> list);`
In regards to `update`, the `void update(const std::string& name);` he
mentioned is a private function.
Since we already have a `calculateShare` this probably could be named
`updateShare`.
---
Some more general thoughts / ideas:
(1) I agree that we don't always want to include all of the parameter
names, but it seems to beneficial in some cases.
It's a bit difficult to accurately characterize the situations, but
one aspect to consider is the likelihood that
the argument will be the same name as the parameter name. If the
argument name will basically always be
the same as the parameter name, we end up with redundancy.
I don't think `map.count(key)` is a good example of this, since the
`key` portion is basically never named `key`.
But perhaps call-sites like `map.count(pid)` and `map.count(name)` are
indicative enough.
The example I want to point out is `void removeFramework(Framework*
framework);`.
Every variable of type `Framework*` we have in the codebase is named
`framework`, and one function that returns
an instance of `Framework*` is called `getFramework`. This means
basically every call-site of `removeFramework`
is either `removeFramework(framework)` or
`removeFramework(getFramework(...));`
(2) I wonder if it would help if we were to "namespace" these functions.
In the case of `removeSlave` for example, perhaps it would help if we
could write something like:
```
master->registry.remove_agent(agent);
master->in_memory.remove_agent(agent, ...);
```
or for the `update` example, to have something like:
```
sorter->clients.update(name, weight);
sorter->clients.calculateShare(name);
sorter->total_resources.add(agent, resources);
sorter->total_resources.update(agent, resources);
sorter->allocations.allocated(name, agent_id, resources);
sorter->allocations.update(name, agent_id, old_allocation,
new_allocation);
```
Example code:
```
class Master {
struct {
void remove_agent(/* ... */) const {
std::cout << self.x << std::endl; // can still access private
members.
}
Master& self;
} registry{*this};
struct InMemory {
void remove_agent(/* ... */) const;
Master& self;
} in_memory{*this};
private:
int x;
};
void Master::InMemory::remove_agent(...) const { ... } // can still
define out-of-line.
```
Just a few thoughts.
MPark
On 3 July 2016 at 21:10, Benjamin Mahler <[email protected]> wrote:
> 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
> >
>
