storage: simplify quota pool deallocation for failed proposals #26979
Conversation
This change simplifies the process of releasing quota back into the quota pool on failed proposals. It does so by pushing all quota management into Raft instead of having the request's goroutine try to determine whether it needs to release quota based on the error in the proposal result. Quota management used to only be handled by Raft for successful proposals. In doing so, the change removes a case where quota could be double freed: proposals that apply and then return an error. This isn't particularly important though, because this pattern is never used anymore. Besides being a simplification, this is important for cockroachdb#26599 because transaction pipelining depends on requests pushing proposals into Raft and then returning immediately. In that model (asyncConsensus) the request's goroutine is not around to release quota resources on failed proposals. Release note: None
|
This change is |
|
Reviewed 4 of 4 files at r1. Comments from Reviewable |
|
@tschottdorf if possible I'd appreciate one more set of eyes who also reviewed the original quota pool change to take a look at this. This stuff is a little hairy to touch, especially because the lifetime of quota reservations aren't exactly the same as that of local proposals (per your comment here, which was in response to a question I also had). |
|
Reviewed 4 of 4 files at r1. pkg/storage/replica.go, line 2813 at r1 (raw file):
Quite happy to see this go. Comments from Reviewable |
|
TFTRs! bors r+ |
Build failed (retrying...) |
Build failed (retrying...) |
|
Spurious failures due to general TeamCity brokenness. bors r+ |
Timed out (retrying...) |
Build failed (retrying...) |
26979: storage: simplify quota pool deallocation for failed proposals r=benesch a=nvanbenschoten This change simplifies the process of releasing quota back into the quota pool on failed proposals. It does so by pushing all quota management into Raft instead of having the request's goroutine try to determine whether it needs to release quota based on the error in the proposal result. Quota management used to only be handled by Raft for successful proposals. In doing so, the change removes a case where quota could be double freed: proposals that apply and then return an error. This isn't particularly important though, because this pattern is never used anymore. Besides being a simplification, this is important for #26599 because transaction pipelining depends on requests pushing proposals into Raft and then returning immediately. In that model (asyncConsensus) the request's goroutine is not around to release quota resources on failed proposals. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
There was a problem hiding this comment.
Reviewable status:
complete! 2 of 0 LGTMs obtained
pkg/storage/replica.go, line 850 at r1 (raw file):
r.mu.AssertHeld() pr := proposalResult{ Err: roachpb.NewError(roachpb.NewAmbiguousResultError("removing replica")),
Moving the error allocation out of the loop was the cause of #31535. Super subtle that this innocuous change would cause a race.
This change simplifies the process of releasing quota back into the
quota pool on failed proposals. It does so by pushing all quota
management into Raft instead of having the request's goroutine
try to determine whether it needs to release quota based on the
error in the proposal result. Quota management used to only be
handled by Raft for successful proposals.
In doing so, the change removes a case where quota could be double
freed: proposals that apply and then return an error. This isn't
particularly important though, because this pattern is never
used anymore.
Besides being a simplification, this is important for #26599 because
transaction pipelining depends on requests pushing proposals into
Raft and then returning immediately. In that model (asyncConsensus)
the request's goroutine is not around to release quota resources on
failed proposals.
Release note: None