Add rate.Limiter to RunConfiguration #65
Conversation
loadgen/generic_executor.go
Outdated
| @@ -25,6 +26,7 @@ type genericRun struct { | |||
| info ScenarioInfo | |||
| config RunConfiguration | |||
| logger *zap.SugaredLogger | |||
| limiter *rate.Limiter | |||
There was a problem hiding this comment.
Why is this repeated here when it's stored on config? In fact, it seems this field is read but never written - I'm not sure anything's actually happening yet.
There was a problem hiding this comment.
🤦 You're totally right. This is a leftover from my initial draft hardcoded implementation.
I've fixed it, now the construction like this works as expected:
loadgen.MustRegisterScenario(loadgen.Scenario{
Description: "Each iteration executes a single workflow with a noop activity.",
Executor: loadgen.KitchenSinkExecutor{
DefaultConfiguration: loadgen.RunConfiguration{
Limiter: rate.NewLimiter(rate.Every(time.Second * 5), 1),
},
TestInput: &kitchensink.TestInput{
WorkflowInput: &kitchensink.WorkflowInput{
InitialActions: []*kitchensink.ActionSet{
kitchensink.NoOpSingleActivityActionSet(),
},
},
},
},
})
There was a problem hiding this comment.
@aromanovich - be advised, if you're using this as a library, the API will never be considered stable and may change in backwards incompatible ways regularly. Even the CLI may not be completely stable, but definitely more so than the API. If possible, you should consider exposing whatever advanced limiting options you need from the CLI.
| var runStartTime time.Time | ||
| err := func() error { | ||
| if g.config.Limiter != nil { | ||
| if innerErr := g.config.Limiter.Wait(ctx); innerErr != nil { | ||
| return innerErr | ||
| } | ||
| } | ||
| runStartTime = time.Now() | ||
| return g.executor.Execute(ctx, run) | ||
| }() |
There was a problem hiding this comment.
| var runStartTime time.Time | |
| err := func() error { | |
| if g.config.Limiter != nil { | |
| if innerErr := g.config.Limiter.Wait(ctx); innerErr != nil { | |
| return innerErr | |
| } | |
| } | |
| runStartTime = time.Now() | |
| return g.executor.Execute(ctx, run) | |
| }() | |
| runStartTime := time.Now() | |
| var err error | |
| if g.config.Limiter != nil { | |
| err = g.config.Limiter.Wait(ctx) | |
| } | |
| if err == nil { | |
| err = g.executor.Execute(ctx, run) | |
| } |
Doesn't seem to be much benefit in the inner function. I don't think we have to overly worry about err reuse.
| err := g.executor.Execute(ctx, run) | ||
| var runStartTime time.Time | ||
| err := func() error { | ||
| if g.config.Limiter != nil { |
There was a problem hiding this comment.
Can this be worked into the part above this goroutine where we do waiting? Seeing max-concurrent, we have traditionally waited in the synchronous loop before instantiating the run.
| @@ -119,6 +123,8 @@ type RunConfiguration struct { | |||
| // Maximum number of instances of the Execute method to run concurrently. | |||
| // Default is DefaultMaxConcurrent. | |||
| MaxConcurrent int | |||
| // Rate limiter to be Wait-ed before each iteration. | |||
| Limiter *rate.Limiter | |||
There was a problem hiding this comment.
| Limiter *rate.Limiter | |
| Limiter interface { Wait(context.Context) error } |
Quite a bit more flexible
| if r.Limiter == nil { | ||
| r.Limiter = DefaultRateLimiter | ||
| } |
There was a problem hiding this comment.
| if r.Limiter == nil { | |
| r.Limiter = DefaultRateLimiter | |
| } |
Seems a nil rate limiter is supported at runtime
| if run.config.Limiter == nil { | ||
| run.config.Limiter = g.DefaultConfiguration.Limiter | ||
| } |
There was a problem hiding this comment.
| if run.config.Limiter == nil { | |
| run.config.Limiter = g.DefaultConfiguration.Limiter | |
| } |
Seems a nil limiter is supported at runtime
| // DefaultRateLimiter is unlimited | ||
| var DefaultRateLimiter = rate.NewLimiter(rate.Inf, 0) | ||
|
|
There was a problem hiding this comment.
| // DefaultRateLimiter is unlimited | |
| var DefaultRateLimiter = rate.NewLimiter(rate.Inf, 0) |
Seems a nil limiter is supported at runtime
What was changed
RunConfigurationnow accepts*rate.Limiter.Why?
To allow spreading out the load surge caused by going from 0 to
MaxConcurrentruns in the beginning of the benchmark.Checklist
No issues to close.
How was this tested:
I ran benchmarks.
Any docs updates needed?
No.