add members_info state query and API wrapper

rabbitmq / ra

A Raft implementation for Erlang and Elixir that strives to be efficient and make it easier to use multiple Raft clusters in a single system.

Other

798 stars 93 forks source link

add members_info state query and API wrapper #413

Closed illotum closed 5 months ago

illotum commented 5 months ago

Proposed Changes

Adds state query and API to return leader's cluster view.

Types of Changes

[ ] Bug fix (non-breaking change which fixes issue #NNNN)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation (correction or otherwise)
[ ] Cosmetics (whitespace, appearance)

Checklist

[x] I have read the CONTRIBUTING.md document
[x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
[x] All tests pass locally with my changes
[x] I have added tests that prove my fix is effective or that my feature works
[ ] I have added necessary documentation (if appropriate)
[ ] Any dependent changes have been merged and published in related repositories

illotum commented 5 months ago

Opening for review.

@kjnilsson what if we called it cluster_info instead? Considering the return type and internal representation, it feels a little more intuitive.

Second question, should I send zeroes instead of no values for the ease of matching? This mainly concerns the corner case of a local query to non-leader. On the flip side, I fear sending anything invalid will lead to user confusion.

michaelklishin commented 5 months ago

@illotum cluster_info is not a bad name but note that a cluster status is (or at least can be) more than the status of its members. members_info is more specific in that sense.

illotum commented 5 months ago

Added a commit to extract membership from voter_status. I'm uncertain if this is better than full voter_status, but clearly it is the main value of that entry. I can think of a situation where target may be valuable, so happy to revert.

Eh. This breaks dyalize again, I'll revert back to full voter_status.

kjnilsson commented 5 months ago

Opening for review.

@kjnilsson what if we called it cluster_info instead? Considering the return type and internal representation, it feels a little more intuitive.

I think the link to ra:members/1 works in terms of it's name so let's keep it as is.

Second question, should I send zeroes instead of no values for the ease of matching? This mainly concerns the corner case of a local query to non-leader. On the flip side, I fear sending anything invalid will lead to user confusion.

I think what we're hitting here is the fact that this query makes less sense as a local query as really you want to issue it against the leader. I suggest when used as a local query we only return infos that make sense to return, e.g. the voter_status. Alt. we always make this query a leader query. Happy to consider either way.

illotum commented 5 months ago

I'm leaning towards returning something. There are cases where one may want to investigate follower's view of the cluster, however limited that is.

And I'll specify in the function doc that local queries are a special case.

michaelklishin commented 5 months ago

In the context of RabbitMQ's needs of determining if a node hosts any "catching up" (non-voter) replicas, either can work but a leader query makes more sense to me.