Skip to content

Commit 436459b

Browse files
committed
admission: replace GrantCoordinator with storeGrantCoordinator
Almost all the functionality of GrantCoordinator was unused and the unnecessary abstractions were confusing. Only the requester's (behind storeRequester, and the kvStoreGranter.*Requester fields) are now abstracted out for testing. The confusing locking is replaced by kvStoreTokenGranter having its own mutex. A few fields were erroneously not inside kvStoreTokenGranter.coordMu (even though they were always accessed using the mutex, so there wasn't a bug), and that is fixed too. The testdata changes are only an elimination of the unnecessary prefix related to grant chaining when printing the state of the GrantCoordinator: there was never any grant chaining for the store GrantCoordinator. Epic: none Release note: None
1 parent 9d40033 commit 436459b

File tree

6 files changed

+362
-309
lines changed

6 files changed

+362
-309
lines changed

pkg/util/admission/admission.go

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,7 @@
5656
// across WorkKinds that is used to reflect their shared need for underlying
5757
// resources.
5858
// - The top-level GrantCoordinator which coordinates grants across these
59-
// WorkKinds. The WorkKinds handled by an instantiation of GrantCoordinator
60-
// will differ for single-tenant clusters, and multi-tenant clusters
61-
// consisting of (multi-tenant) KV nodes and (single-tenant) SQL nodes.
59+
// WorkKinds, for CPU.
6260
//
6361
// The interfaces involved:
6462
// - requester: handles all requests for a particular WorkKind. Implemented by
@@ -75,8 +73,7 @@
7573
// the lock in WorkQueue).
7674
// - cpuOverloadIndicator: this serves as an optional additional gate on
7775
// granting, by providing an (ideally) instantaneous signal of cpu overload.
78-
// The kvSlotAdjuster is the concrete implementation, except for SQL
79-
// nodes, where this will be implemented by sqlNodeCPUOverloadIndicator.
76+
// The kvSlotAdjuster is the concrete implementation.
8077
// CPULoadListener is also implemented by these structs, to listen to
8178
// the latest CPU load information from the scheduler.
8279
//
@@ -97,7 +94,7 @@
9794
// that is setup here.
9895
//
9996

100-
// Partial usage example (regular cluster):
97+
// Partial usage example:
10198
//
10299
// var metricRegistry *metric.Registry = ...
103100
// coord, metrics := admission.NewGrantCoordinator(admission.Options{...})
@@ -217,16 +214,17 @@ type granter interface {
217214
}
218215

219216
// granterWithLockedCalls is an encapsulation of typically one
220-
// granter-requester pair, and for kvStoreTokenGranter of two
221-
// granter-requester pairs (one for each workClass). It is used as an internal
217+
// granter-requester pair. It is used as an internal
222218
// implementation detail of the GrantCoordinator. An implementer of
223219
// granterWithLockedCalls responds to calls from its granter(s) by calling
224220
// into the GrantCoordinator, which then calls the various *Locked() methods.
225221
// The demuxHandle is meant to be opaque to the GrantCoordinator, and is used
226222
// when this interface encapsulates multiple granter-requester pairs -- it is
227-
// currently used only by kvStoreTokenGranter, where it is a workClass. The
223+
// currently unused and will be removed. The
228224
// *Locked() methods are where the differences in slots and various kinds of
229225
// tokens are handled.
226+
//
227+
// TODO(sumeer): remove the demuxHandle.
230228
type granterWithLockedCalls interface {
231229
// tryGetLocked is the real implementation of tryGet from the granter
232230
// interface. demuxHandle is an opaque handle that was passed into the

pkg/util/admission/grant_coordinator.go

Lines changed: 2 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,6 @@ type GrantCoordinator struct {
8686
// close().
8787
queues [numWorkKinds]requesterClose
8888

89-
ioLoadListener *ioLoadListener
90-
9189
// See the comment at continueGrantChain that explains how a grant chain
9290
// functions and the motivation. When !useGrantChains, grant chains are
9391
// disabled.
@@ -275,46 +273,6 @@ func makeRegularGrantCoordinator(
275273
return coord
276274
}
277275

278-
// pebbleMetricsTick is called every adjustmentInterval seconds and passes
279-
// through to the ioLoadListener, so that it can adjust the plan for future IO
280-
// token allocations.
281-
func (coord *GrantCoordinator) pebbleMetricsTick(ctx context.Context, m StoreMetrics) bool {
282-
return coord.ioLoadListener.pebbleMetricsTick(ctx, m)
283-
}
284-
285-
// allocateIOTokensTick tells the ioLoadListener to allocate tokens.
286-
func (coord *GrantCoordinator) allocateIOTokensTick(remainingTicks int64) {
287-
coord.ioLoadListener.allocateTokensTick(remainingTicks)
288-
coord.mu.Lock()
289-
defer coord.mu.Unlock()
290-
if !coord.mu.grantChainActive {
291-
coord.tryGrantLocked()
292-
}
293-
// Else, let the grant chain finish. NB: we turn off grant chains on the
294-
// GrantCoordinators used for IO, so the if-condition is always true.
295-
}
296-
297-
// adjustDiskTokenError is used to account for errors in disk read and write
298-
// token estimation. Refer to the comment in adjustDiskTokenErrorLocked for more
299-
// details.
300-
func (coord *GrantCoordinator) adjustDiskTokenError(m StoreMetrics) {
301-
coord.mu.Lock()
302-
defer coord.mu.Unlock()
303-
if storeGranter, ok := coord.granters[KVWork].(*kvStoreTokenGranter); ok {
304-
storeGranter.adjustDiskTokenErrorLocked(m.DiskStats.BytesRead, m.DiskStats.BytesWritten)
305-
}
306-
}
307-
308-
// testingTryGrant is only for unit tests, since they sometimes cut out
309-
// support classes like the ioLoadListener.
310-
func (coord *GrantCoordinator) testingTryGrant() {
311-
coord.mu.Lock()
312-
defer coord.mu.Unlock()
313-
if !coord.mu.grantChainActive {
314-
coord.tryGrantLocked()
315-
}
316-
}
317-
318276
// GetWorkQueue returns the WorkQueue for a particular WorkKind. Can be nil if
319277
// the NewGrantCoordinator* function does not construct a WorkQueue for that
320278
// work.
@@ -594,13 +552,8 @@ func (coord *GrantCoordinator) SafeFormat(s redact.SafePrinter, _ rune) {
594552
switch g := coord.granters[i].(type) {
595553
case *slotGranter:
596554
s.Printf("%s%s: used: %d, total: %d", curSep, kind, g.usedSlots, g.totalSlots)
597-
case *kvStoreTokenGranter:
598-
s.Printf(" io-avail: %d(%d), disk-write-tokens-avail: %d, disk-read-tokens-deducted: %d",
599-
g.coordMu.availableIOTokens[admissionpb.RegularWorkClass],
600-
g.coordMu.availableIOTokens[admissionpb.ElasticWorkClass],
601-
g.coordMu.diskTokensAvailable.writeByteTokens,
602-
g.coordMu.diskTokensError.diskReadTokensAlreadyDeducted,
603-
)
555+
default:
556+
s.Printf("unknown granter")
604557
}
605558
case SQLKVResponseWork, SQLSQLResponseWork:
606559
if coord.granters[i] != nil {

0 commit comments

Comments
 (0)