Add quotas to Permits #3333
Add quotas to Permits #3333
Conversation
Allows a Permit impl to decide how much quota a caller is allowed to use for each Permit acquired. Having a quota > 1 is a nice performance optimization to avoid having to acquire a permit everytime a resource is consumed. Additionally the new Permit structure allows tracking if a Permit is acquired/released more than once by a caller using a refCount. Allows catching bugs in tests by panicing.
Codecov Report
@@ Coverage Diff @@
## master #3333 +/- ##
=======================================
Coverage 72.5% 72.5%
=======================================
Files 1100 1100
Lines 103053 103053
=======================================
Hits 74768 74768
Misses 23149 23149
Partials 5136 5136
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| @@ -939,17 +937,11 @@ type fetchTaggedResultsIterOpts struct { | |||
| } | |||
|
|
|||
| func newFetchTaggedResultsIter(opts fetchTaggedResultsIterOpts) FetchTaggedResultsIter { //nolint: gocritic | |||
| _, limitBased := opts.blockPermits.(*permits.LookbackLimitPermit) | |||
There was a problem hiding this comment.
So nice to be able to get rid of this
| if i.idx == idx { | ||
| // block acquiring if we need the block readers to fulfill the current fetch. | ||
| if err := i.blockPermits.Acquire(ctx); err != nil { | ||
| permit, err := i.blockPermits.Acquire(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
interesting; so if we do run into an error, we don't technically release all held permits? Or is that handled elsewhere? (separate comment from your change I think)
There was a problem hiding this comment.
when an error happens, the fetched tagged iterator is closed, which releases all held permits.
|
|
||
| // mutable state | ||
| quotaUsed int64 | ||
| refCount atomic.Int32 |
There was a problem hiding this comment.
can we call out that this should be 0 or 1?
| } | ||
|
|
||
| // Permit is granted to a caller which is allowed to consume some amount of quota. | ||
| type Permit interface { |
There was a problem hiding this comment.
hmm, callers should never invoke release or acquire, fair to say? I guess the panics help guard that?
There was a problem hiding this comment.
correct. I couldn't think of a better name. maybe PrepareForRelease and PrepareForAcquire?
There was a problem hiding this comment.
or maybe PostRelease and PreAcquire, kind of like hooks for the managers to call
* master: (22 commits) Remove deprecated fields (#3327) Add quotas to Permits (#3333) [aggregator] Drop messages that have a drop policy applied (#3341) Fix NPE due to race with a closing series (#3056) [coordinator] Apply auto-mapping rules if-and-only-if no drop policies are in effect (#3339) [aggregator] Add validation in AddTimedWithStagedMetadatas (#3338) [coordinator] Fix panic in Ready endpoint for admin coordinator (#3335) [instrument] Config option to emit detailed Go runtime metrics only (#3332) [aggregator] Sort heap in one go, instead of iterating one-by-one (#3331) [pool] Add support for dynamic, sync.Pool backed, object pools (#3334) Enable PANIC_ON_INVARIANT_VIOLATED for tests (#3326) [aggregator] CanLead for unflushed window takes BufferPast into account (#3328) Optimize StagedMetadatas conversion (#3330) [m3msg] Improve message scan performance (#3319) [dbnode] Add reason tag to bootstrap retries metric (#3317) [coordinator] Enable rule filtering on prom metric type (#3325) Update m3dbnode-all-config.yml (#3204) [coordinator] Include Type in RollupOp.Equal (#3322) [coordinator] Simplify iteration logic of matchRollupTarget (#3321) [coordinator] Add rollup type to remove specific dimensions (#3318) ...
| processingTime := time.Since(startProcessing) | ||
| queryMetrics.blockProcessingTime.RecordDuration(processingTime) | ||
| blockIter.processingTime += processingTime | ||
| permits.Release(int64(processingTime)) | ||
| permit.Use(int64(processingTime)) |
There was a problem hiding this comment.
@ryanhall07 What's the motivation for calling .Use on the permit, if a line later it is immediately released, hence you're not using that information anywhere?
There was a problem hiding this comment.
so the Permit can track how much of the allowed quota was actually used. impls of the Permits may care to know about this information.
Allows a Permit impl to decide how much quota a caller is allowed to use
for each Permit acquired. Having a quota > 1 is a nice performance
optimization to avoid having to acquire a permit everytime a resource is
consumed.
Additionally the new Permit structure allows tracking if a Permit is
acquired/released more than once by a caller using a refCount. Allows
catching bugs in tests by panicing.
What this PR does / why we need it:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: