-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return status of set_priority. Ability to broadcast priority changes if there's a leader #432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @myrrc, thanks for submitting this PR.
I understand how it works, and the overall code looks good to me, but can you please elaborate on why (and in which case) broadcast_when_leader_exists
option is needed? Enforcing priority even with a live leader will cause cluster config to diverge, which should be strictly avoided (it only affects priority but not data though).
Also, please break lines at 90 chars. There are a few places to fix.
Hi @greensky00.
This PR is needed for my other PR regarding a ZooKeeper-like program. Each ZooKeeper-like replica
has a NuRaft server inside, and it can be sent ZooKeeper-compatible commands. When a command gets
to the replica, replica must process it or abort. For `add/remove_srv` we can check whether result was
accepted. For `set_priority` we don't have this guarantee.
What should we do if we don't want to lose the priority update message? We can't rely on leadership and
only process message at the current leader as:
- there is a chance there will be elections after returning from `is_leader()` and before appending new config
to log in `set_priority` in which case we'll lose the update.
- there is a counter-example on {A, B, C} cluster where command going to A sees B as leader,
then there are re-elections and command on B sees C as leader,
re-elections again, and then command on C sees A as leader.
In this case changes would also be lost.
Nor can we simply change our config via `get_server(id)->set_priority` as NuRaft has various invariants inside
like `uncommitted_config_` that would be broken.
So, we need to `set_priority` on each replica explicitly. However, if there's a live leader,
unlike `add/remove_srv`, the new config won't be forwarded (I tried implementing this but failed as it
seems quite a big task) and we'll lose the update.
Thus, a new option is introduced that would send messages to replicas telling to update priority even when a
leader is present. In my case there are no chances of going out of sync (as update will be propagated to every
replica) but I agree it can happen in general. Please note, however, that invoking `set_priority` when
there's no live leader currently can also lead to inconsistencies as a peer can reject the priority update
message (`is_bisy()` case).
I will address formatting issues and documentation for `set_priority` today.
Kind regards,
Mike.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
set_priority
is a bit inconsistent withadd_srv
andremove_srv
.If you invoke the latter two on a follower, the request will be
forwarded to leader. You can wait on the request and check whether it was accepted or not.
Unfortunately, there are no such guarantees for
set_priority
.If you invoke it on a follower, and there is a live leader, function would quit.
I want to know the result of
set_priority
, so changes are proposed that change its return type.