-
Notifications
You must be signed in to change notification settings - Fork 801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Matching simulation improvements #6224
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -176,31 +181,35 @@ func (s *MatchingSimulationSuite) TestMatchingSimulation() { | |||
numPollers := getNumPollers(s.testClusterConfig.MatchingConfig.SimulationConfig.NumPollers) | |||
pollDuration := getPollDuration(s.testClusterConfig.MatchingConfig.SimulationConfig.PollTimeout) | |||
polledTasksCounter := int32(0) | |||
maxTasksToGenerate := getMaxTaskstoGenerate(s.testClusterConfig.MatchingConfig.SimulationConfig.MaxTaskToGenerate) | |||
var tasksToReceive sync.WaitGroup | |||
tasksToReceive.Add(maxTasksToGenerate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simulation runs for 180s at most (timeout set here) and maxTasksToGenerate
might not be hit if it's set too high. please add a comment in the test code here about this to help identify "simulation is stuck" cases due to misconfiguration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks.
taskgeneratortickinterval: 10ms | ||
maxtasktogenerate: 1500 | ||
polltimeout: 5s | ||
forwardermaxoutstandingpolls: 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default setting in prod is 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just adding this as an example for now, I'll be adding a full suite that we can use to validate it.
Fix sync matching by increasing the deadline for AddDecisionTask. Sync matching subtracts 1 second from the deadline to determine how long to spend sync matching, and with a deadline of 0.5 seconds it immediately times out. Add support for parameterizing the configuration file to use. This will allow us to have several scenarios that we evaluate any matching changes against. Rather than running for a fixed time, run until the expected number of tasks have been consumed. This allows for the benchmark to be used as a measure of throughput and also makes it run faster for short lived scenarios.
Fix sync matching by increasing the deadline for AddDecisionTask. Sync matching subtracts 1 second from the deadline to determine how long to spend sync matching, and with a deadline of 0.5 seconds it immediately times out.
Add support for parameterizing the configuration file to use. This will allow us to have several scenarios that we evaluate any matching changes against.
Rather than running for a fixed time, run until the expected number of tasks have been consumed. This allows for the benchmark to be used as a measure of throughput and also makes it run faster for short lived scenarios.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes