Shard OPS limiter #1636
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1636
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/shard_io_limiter"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A limiter is used to limit the rate of shard operations.
If resources are exceeded, a special error is returned.
shard.limits
config6686448d86
to619a115f66
619a115f66
toa3e271cc23
a3e271cc23
to77f67f837d
44218bf4f6
tobb4241472d
bb4241472d
toac30e6f324
ac30e6f324
tof383ab1fe6
f383ab1fe6
toc82056f963
c82056f963
toda05997a6d
da05997a6d
to3f7ef8ee01
3f7ef8ee01
to519cf71a27
6da33cd724
to9553f19670
9553f19670
to8b7517cb6d
8b7517cb6d
to5453747d80
5453747d80
to5e60db6705
5e60db6705
to083299742f
083299742f
toab5b48e28b
ab5b48e28b
to49c384ef9d
49c384ef9d
tof22519cef7
WIP: Shard OPS limiterto Shard OPS limiter@ -125,6 +126,14 @@ func (x *Config) GC() *gcconfig.Config {
)
}
// Limits returns "limits" subsection as a gcconfig.Config.
gcconfig?
fixed
fixed
@ -139,7 +139,10 @@
"skip_session_token_issuer_verification": true
},
"get": {
"priority": ["$attribute:ClusterName", "$attribute:UN-LOCODE"]
Please revert it
fixed
@ -303,1 +304,4 @@
### `limits` subsection
### `limits` subsection
You defined this section twice
fixed
fixed
@ -0,0 +57,4 @@
return &semaphore{limit: int64(config.MaxRunningOps)}, nil
}
return scheduling.NewMClock(
uint64(config.MaxRunningOps), uint64(config.MaxWaitingOps),
You do a cast from
int64
toint
when reading a config, then you castint
touint64
here. What about makingconfig.MaxRunningOps
andconfig.MaxWaitingOps
of typeuint64
?Fixed: now config variables have int64 type.
Fixed: now config variables have int64 type.
f22519cef7
to865c919bda
865c919bda
toa9d777da11
@ -372,0 +381,4 @@
return err
}
if newConfig.limiter != nil {
newConfig.limiter.Close()
You close a limiter of the new config is closed, not the old one. Is it right?
It is right. Performed refactoring: renamed
newConfig
->target
,oldConfig
->source
.@ -0,0 +40,4 @@
}
readScheduler, err := createScheduler(c.Read())
if err != nil {
return nil, fmt.Errorf("failed to create read scheduler: %w", err)
Should be without "failed to"
ok, fixed
@ -0,0 +78,4 @@
return fmt.Errorf("invalid limit_ops for tag %s: must be positive value", t.String())
}
if float64Value(v.Reserved) < 0 || math.IsNaN(float64Value(v.Reserved)) {
return fmt.Errorf("invalid limit_ops for tag %s: must be positive value", t.String())
limit_ops
->reserved_ops
fixed
a9d777da11
tocef12da660
dstepanov-yadro referenced this pull request2025-02-24 08:41:06 +00:00
3e319e6789
to1ef6b5c4cd
@ -0,0 +71,4 @@
MaxRunningOps int64
// IdleTimeout returns the value of "idle_timeout" config parameter.
//
// Returns DefaultIdleTimeout if the value is not a valid duration.
Struct field doesn't return anything. Sth about default value here?
Fixed
@ -0,0 +2,4 @@
import "strings"
func Cond(cond bool, details ...string) {
Don't mind the naming, but do you have any examples of it being used somewhere?
assert.True
is what I have seen in e.g. testify library.Fixed
@ -0,0 +2,4 @@
import "strings"
func Cond(cond bool, details ...string) {
Btw, good call with introducing this function!
@ -0,0 +27,4 @@
default:
}
v := s.count.Add(1)
I have seen similar code in
frostfs-qos/limiting/semaphore
by @a-savchuk , have you not reused it deliberately?I just didn't wait for merge.
Fixed.
@ -195,0 +175,4 @@
type rebuildLimiter struct {
concurrencyLimiter common.ConcurrencyLimiter
rateLimiter qos.Limiter
I think rebuild test (and maybe all tests, acutally) should check that limiter is in some ground state after tests.
Current limiter API makes it possible to forget calling release.
WIth tests we can ensure that it is called at least in the code we have tested.
Hm, may be we can do this in
storageEngine.Close()
?It is already called in
Cleanup()
and after allwg.Wait()
I would expect limiter to be empty.The problem is that there we have an interface, casting looks nice only in tests.
Added validation to rebuild tests.
I hope that there will be no such cases, because the compiler should highlight this as an unused variable.
Also added validation to engine tests.
Hm, good point
1ef6b5c4cd
to9b91325160
fa48aabdf3
to5740fe3418
5740fe3418
to5e6764b51e
5e6764b51e
to303490c34e
303490c34e
to4685afb1dc
New commits pushed, approval review dismissed automatically according to repository settings
awesome