storage: proactively transfer Raft leadership before doing a lease transfer#8837
storage: proactively transfer Raft leadership before doing a lease transfer#8837andreimatei wants to merge 6 commits intocockroachdb:masterfrom
Conversation
The effect is that an error detected by replica.ExecuteBatch, in particular "key out of range" where the batch's header has keys that are not part of the range any more, was swallowed and then something panicked because there was neither an error, nor a BatchResult. But all this only for IsSingleNonKVRequest batches, and moreover, only for read-only batches. We don't currently have read-only non-kv requests, so I can't test this yet, but a test is two commits over in this PR.
and always fill in the originating replica.
To be used for the upcoming LeaseInfo request.
LeaseInfoRequest is a read command for getting the current lease holder for a range. It will be used to proactively refresh the LeaseHolderCache when SQL planning needs to figure out some lease holders.
Tests the fix from the first commit of this PR.
…ansfer references cockroachdb#8816 There's a period of unavailability between the time the old lease holder has proposed the lease transfer and the time the new lease holder applies it, during which requests for the range float around the cluster looking for a lease holder (see cockroachdb#8816). So, we attempt to move Raft leadership early, making it more likely that the next lease holder applies the lease transfer quickly after it's proposed. This should minimize the unavailability period.
|
only the last commit is to be reviewed here; the rest are https://reviewable.io/reviews/cockroachdb/cockroach/8752 |
|
This sounds scary to me, but I'm not going to have time to give this the review it deserves until after the code yellow. |
|
@bdarnell As you pointed out in the other thread, I also think that we I do think this is useful in spirit, but also want to post-yellow this. On Fri, Aug 26, 2016 at 2:52 AM Ben Darnell notifications@github.com
-- Tobias |
|
Assigned |
|
This change deliberately moves raft leadership away from the lease holder. As such, it will always be at odds with changes like #12323, which try to more reliably bring the two together. I'm also concerned that by transferring raft leadership away before we propose our TransferLease command, we increase the total period of unavailability and the risk of something going wrong (the TransferLease command will sit in the pending queue and will have to be reproposed after the election resolves). I don't think we should merge this change in its current form and we should look for another solution to #8816. Review status: 0 of 19 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
Can't we reconcile this with #12323 by not transferring the raft leadership away from a soon-to-be leaseholder? The old leaseholder knows that a transfer is in progress... I'm not sure about your point on increasing the total period of unavailability. Do you the period from when the lease transfer is started to when the new leaseholder has applied it? |
But the new leaseholder doesn't know it yet, so it would still step down as soon as it gets leadership. There probably is some way we could reconcile these changes, but in general it seems like trouble to introduce a change that deliberately separates lease and leadership. We'll always have to keep an eye on it and how it interacts with our other leadership-transfer logic.
From the time the old leader/leaseholder processes its last command to the time the new one processes its first command. Either way, there's a raft election and a TransferLease command to be committed. These cannot occur in parallel because commands can only be committed when there is a raft leader. When TransferLease is first, we have the problems in #8816. This change puts the raft election first, and I think that leads to at least one more round trip before everything resolves (I'm not sure whether it would slow things down, but it definitely wouldn't reduce the amount of work to be done here) |
|
All right, seems like maybe this isn't such a great change. |
references #8816
There's a period of unavailability between the time the old lease holder
has proposed the lease transfer and the time the new lease holder
applies it, during which requests for the range float around the cluster
looking for a lease holder (see #8816). So, we attempt to move Raft
leadership early, making it more likely that the next lease holder
applies the lease transfer quickly after it's proposed. This should
minimize the unavailability period.
cc @tschottdorf @bdarnell
This change is