storage: allow acquisition from closed quota pools #16413
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
tbg
approved these changes
Jun 9, 2017
pkg/storage/quota_pool_test.go
Outdated
| @@ -122,14 +121,14 @@ func TestQuotaPoolClose(t *testing.T) { | |||
| errCh <- qp.acquire(ctx, 1) | |||
There was a problem hiding this comment.
Might as well have five of those to test a little more.
pkg/storage/quota_pool_test.go
Outdated
| @@ -122,14 +121,14 @@ func TestQuotaPoolClose(t *testing.T) { | |||
| errCh <- qp.acquire(ctx, 1) | |||
| }() | |||
|
|
|||
| qp.close() | |||
| qp.broadcast() | |||
There was a problem hiding this comment.
call it twice just to prove that you can.
pkg/storage/quota_pool_test.go
Outdated
| @@ -122,14 +121,14 @@ func TestQuotaPoolClose(t *testing.T) { | |||
| errCh <- qp.acquire(ctx, 1) | |||
| }() | |||
|
|
|||
| qp.close() | |||
| qp.broadcast() | |||
|
|
|||
| select { | |||
There was a problem hiding this comment.
Would also run 5x if you have 5 acquisitions above.
irfansharif
force-pushed
the
quota-leaseholder
branch
from
5ca446c to
913589a
Compare
|
Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions. pkg/storage/quota_pool_test.go, line 121 at r1 (raw file): Previously, tschottdorf (Tobias Schottdorf) wrote…
Done. pkg/storage/quota_pool_test.go, line 124 at r1 (raw file): Previously, tschottdorf (Tobias Schottdorf) wrote…
lol, Done. pkg/storage/quota_pool_test.go, line 126 at r1 (raw file): Previously, tschottdorf (Tobias Schottdorf) wrote…
Done. Comments from Reviewable |
guess not, renamed. |
(actually) Fixes cockroachdb#16376, reverts cockroachdb#16399. TestRaftRemoveRace touched the short window of time where it was possible that the lease holder and the raft leader were not the same replica (raft leadership could change from under us, but the lease holder stayed steady). Consider the following sequence of events: - the lease holder and the raft leader are co-located - 'add replica' commands get queued up on the replicate queue - leader replica steps down as leader thus closing the quota pool (on the leaseholder, because they're one and the same) - commands get out of the queue, cannot acquire quota because the quota pool is closed (on the lease holder) and fail with an error indicating so We make two observations: - quotaPool.close() only takes place when a raft leader is becoming a follower and thus causing all ongoing acquisitions to fail - Ongoing acquisitions are only taking place on the lease holder replica The quota pool was implemented in a manner such that it is effectively disabled when the lease holder and the range leader are not co-located. Failing with an error here (now that the raft leader has changed, the lease holder and raft leader are no longer co-located) runs contrary to this. What we really want is to "fail open" in this case instead, i.e. allow the acquisition to proceed as if the quota pool is effectively disabled.
irfansharif
force-pushed
the
quota-leaseholder
branch
from
913589a to
5f8d7a8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(actually) Fixes #16376, reverts #16399.
TestRaftRemoveRacetouched the short window of time where it waspossible that the lease holder and the raft leader were not the same
replica (raft leadership could change from under us, but the lease
holder stayed steady).
Consider the following sequence of events:
the leaseholder, because they're one and the same
closed (on the lease holder) and fail with an error indicating so
We make two observations:
quotaPool.close()only takes place when a raft leader is becoming afollower and thus causing all ongoing acquisitions to fail
The quota pool was implemented in a manner such that it is effectively
disabled when the lease holder and the range leader are not co-located.
Failing with an error here (now that the raft leader has changed, the
lease holder and raft leader are no longer co-located) runs contrary to this.
What we really want is to "fail open" in this case instead, i.e. allow the
acquisition to proceed as if the quota pool is effectively disabled.