Refactor ramping VUs executor #2155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0dfb4c7
to
58d14cd
Compare
imiric
previously approved these changes
Oct 14, 2021
Thanks! I actually automatically ran the test 1000 times to make sure that it's not flaky. |
yorugac
reviewed
Oct 19, 2021
58d14cd
to
ef7063a
Compare
codebien
requested changes
Oct 19, 2021
ef7063a
to
2f80991
Compare
yorugac
previously approved these changes
Oct 21, 2021
codebien
previously approved these changes
Oct 21, 2021
na--
requested changes
Oct 22, 2021
|
Thanks, @na-- 😍 I’ll fix these today. |
2ce9b73
to
5157a36
Compare
5157a36
to
2350dda
Compare
yorugac
requested changes
Nov 17, 2021
RampingVUs.Run method was complex and this refactors it by adding
rampingVUsRunState to share run state between functions used by
the Run method.
+ Makes the Run method easier to read and understand
+ Makes it explicit which Goroutines are being fired in the Run
+ Separates responsibilities to other parts like:
+ rampingVUsRunState and its methods.
+ Moves trackProgress Goroutine from the makeProgressFn
to the Run method
+ Makes initializeVUs to only handle initializing GRs
+ Uses two strategy functions to make them reusable
and clearer. handleVUs and handleRemainingVUs use them.
+ Makes the handleVUs algorithm easier to understand and
manage.
This test aims to check whether the ramping VU executor interrupts hanging/remaining VUs after the graceful rampdown period finishes. handleRemainingVUs method should be interrupting the VUs that are beyond their allowed time budgets. See the comments in the test for more information.
This commit uses longer variables names to help understand code.
handleVUs and handleRemainingVUs was not sharing the current number of planned VUs in graceful steps. This may cause the latter method to act strangely. It's because handleRemainingVUs needs to know the number of remaining graceful steps so that it can stop the remaining VUs accordinly. Now the handleVUs method returns the number of handled graceful steps. So the handleRemainingVUs method can use this information and hard stop the remaining VUs.
2350dda
to
a9b039f
Compare
mstoykov
reviewed
Nov 18, 2021
+ Also changes the value receiver type to a pointer receiver for rampingVUsRunState.
95fb52d
to
4bb561d
Compare
yorugac
approved these changes
Nov 22, 2021
|
At last, thanks! 😅 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RampingVUs.Runmethod is complex and this PR aims to refactor it.Improvements:
Runmethod easier to read and understand.Runmethod.rampingVUsRunStateand its methods.trackProgressGoroutine from themakeProgressFnto theRunmethod.populateVUHandlesto only handle initializing VU handles (not firing up a GR)handleVUsandhandleRemainingVUsuse them).handleVUsalgorithm easier to understand and manage.handleRemainingVUstest with explanations to make newcomers' and new contributors' job easierNote: The other PR (#2124) is convoluted and somehow creates problems locally and with Github. I'd like to keep working on it on this separate branch.