Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

pkg/storage/replica.go, line 295 at r2 (raw file):

		//
		// The *ProposalData in the map are "owned" by it. Elements from the
		// map must only be referenced while the Replica.raftMu is held, except

@nvanbenschoten and I spoke offline about this. It intended to say the Replica.mu but we added further synchronization on the ctx field in #38343. I added some more comments but I'm not sure they're super clear. I'm going to work on moving the proposals map down below the raftMu instead.

very nice!

Reviewed 3 of 12 files at r1, 10 of 10 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

pkg/storage/replica_destroy.go, line 157 at r3 (raw file):

// It requires that both mu and raftMu are held.
func (r *Replica) cancelPendingCommandsLocked() {
	r.raftMu.AssertHeld()

nit: assert in the same order that you do above.

pkg/storage/replica_proposal_quota.go, line 244 at r3 (raw file):

		var toRelease *quotapool.IntAlloc
		for _, rel := range r.mu.quotaReleaseQueue[:numReleases] {
			if rel == nil || !rel.From(r.mu.proposalQuota) {

Add a comment about when !rel.From(r.mu.proposalQuota) would evaluate to true.

pkg/storage/replica_proposal_quota.go, line 250 at r3 (raw file):

				toRelease = rel
			} else {
				toRelease.Merge(rel)

Is this more efficient than releasing each IntAlloc one-by-one?

TFTR! I reworked the release logic into the quotapool package.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @nvanbenschoten)

pkg/storage/replica_destroy.go, line 157 at r3 (raw file):

Switched the other one.

pkg/storage/replica_proposal_quota.go, line 244 at r3 (raw file):

Added an NB but also moved this logic into the quotapool package as a separate commit.

pkg/storage/replica_proposal_quota.go, line 250 at r3 (raw file):

Yes. Merge updates the value in toRelease and puts rel back into the underlying sync.Pool without any synchronization. Release needs to synchronize on the Pool to check if somebody needs to get notified.

pkg/storage/replica_proposal_quota.go, line 141 at r5 (raw file):

			// (if any) and release the quotaReleaseQueue so its allocs are pooled.
			r.mu.proposalQuota.Close("leader change")
			r.mu.proposalQuota.Release(r.mu.quotaReleaseQueue...)

I had forgotten to release this before.

Reviewed 1 of 13 files at r4, 12 of 12 files at r6, 11 of 11 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

pkg/storage/replica_proposal_quota.go, line 250 at r3 (raw file):

👍 thanks for encapsulating this concern.

pkg/storage/replica_proposal_quota.go, line 141 at r5 (raw file):

Good catch. Did you hit an assertion or anything?

pkg/util/quotapool/intpool_test.go, line 473 at r6 (raw file):

}

// TestIntpoolRelease tests the Release method of intpool to ensure that it releases

Is there more to this or is it just missing a period?

pkg/util/quotapool/intpool_test.go, line 507 at r6 (raw file):

			pools := makePools()
			allocs := make([]*quotapool.IntAlloc, len(c.toAcquire))
			for i, acq := range c.toAcquire {

Let's require that the fields in acq are within their numPools and capacity limits.

TFTR

bors r=nvanbenschoten

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @nvanbenschoten)

pkg/storage/replica_proposal_quota.go, line 141 at r5 (raw file):

No. It wasn't a correctness issue, just weren't pooling.

pkg/util/quotapool/intpool_test.go, line 473 at r6 (raw file):

Finished the sentence.

pkg/util/quotapool/intpool_test.go, line 507 at r6 (raw file):

It will panic if it's outside of numPools. Requests above capacity are truncated so it's not technically a problem but added an assertion anyway.

Reviewed 1 of 12 files at r8, 11 of 11 files at r9.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei)