basho / riak_core

Distributed systems infrastructure used by Riak.
Apache License 2.0
1.23k stars 392 forks source link

Maybe improve documentation for size/1 in chash #986

Closed Raphexion closed 2 years ago

Raphexion commented 2 years ago

While reading the source code for chash I found the documentation for the size/1 function a little confusing. The comment say that it returns the number of partitions, and the code returns the number of nodes.

This is probably just confusing because I know so little. But I wanted to ask because it might be a mistake.

https://github.com/basho/riak_core/blob/develop/src/chash.erl#L175-L179

%% @doc Return the number of partitions in the ring.
-spec size(CHash :: chash()) -> integer().
size(CHash) ->
    {_NumPartitions,Nodes} = CHash,
    length(Nodes).
martinsumner commented 2 years ago

It is confusing.

The type definition for chash() is:

https://github.com/basho/riak_core/blob/develop-3.0/src/chash.erl#L61

So the second part is [node_entry()] not [node()]. The node_entry() definition is:

https://github.com/basho/riak_core/blob/develop-3.0/src/chash.erl#L70

So this is a list of tuples, a mapping between partition and node - so that list is as long as the number of partitions in the cluster, not the number of nodes. So chash:size/1 does return the number of partitions not the number of nodes, so the doc is correct, just the naming in the code (use of Nodes) is confusing.

Hence why it is used here: https://github.com/basho/riak_core/blob/develop-3.0/src/riak_core_ring.erl#L481-L483

What I'm not clear about is why it calculates this length, rather than trusting the first element of the tuple, which is just the number of partitions. I don't know of a circumstance where this may be different, but perhaps there is some peculiarity here that requires it to be this way.

P.S. If you're interested in learning more about riak_core, check out the riak_core_lite repo. The riak_core_lite team maintain tutorials, to help people learn how to use riak_core. Also, because the code has no use in pre-existing applications (e.g. Riak), they've been able to be a lot more aggressive at cleaning up the code and making it easier to follow.

Raphexion commented 2 years ago

@martinsumner Thank you for such a detailed answer :sunny: .

Thank you so much for the link to riak_core_lite. I am very interested in learning more about the riak_core and what makes it work. This is exactly what I am looking for.

Concerning the documentation. I guess it is safest to just leave it as it is. My initial feeling was exactly like you wrote, why not just trust the NumPartitions variable. But as you write, the code is most likely written like it is, for a reason. Better not mess with it.

I will close this issue. And thank you again for the detailed answer.