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.
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
808/about executors #974
808/about executors #974
Changes from 12 commits
bb72e32
f970d1a
702149a
d46d5bb
6bddcc5
94aa2d9
99aacd9
c92b6dd
0c12d52
8596a32
ed10eb2
cb02cef
2becf58
b547cbc
020260c
b068f6f
98a361d
872b78f
c658116
4b7b088
977c45d
f580420
5fa0a38
6c0e924
11e7c43
19784d4
5c6f9f8
9aaf8b8
714d037
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 am not sure I understand "design better tests for resources and goals"
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 thinking was that if you understand scenarios better, you can:
shared iterations
is a simpler choice.Of course now I realize that this is an enormous amount of implied information.
Will change to:
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.
"VU allocation" seems a bit misleading, since we are only talking about arrival-rate scenarios
or maybe even
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.
Need to think about this.
"Arrival-rate configuration" probably opens the door to more topics, like using options. That's not bad, but it may mean there's a better place to put this (not blocking for this PR). Is there any reason readers should know about non-arrival rate allocation? If so, maybe we could add it. If not, maybe it doesn't matter to document.
Not sure. It's a good point though.
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.
Now that I've thought about it, I don't want to go with "Arrival-rate configuration" because that should include Graceful stop and maybe more, which opens a whole new round of content structure. It's nice to keep info atomic.
Is VU allocation in non-arrival-rate ever important? If so, we could just add it to the doc later.
Either way, I choose these new titles, ranked by preference. You can pick and that's what will go with:
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.
graceful stop is not specific to arrival-rate executors and already has its own dedicated page that explains it: https://k6.io/docs/using-k6/scenarios/graceful-stop/
Well, considering the configuration of non-arrival-rate executors is specified in terms of VUs, there isn't really anything complicated to explain there 😅
Again, the complexity with arrival-rate is not just how VUs are allocated, but how to balance the VUs and the desired rate and how to find the right values for the former based on the latter and on iteration duration.
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.
Is this not encompassed in
VU Pre-allocation
? Basically, I'm looking for the shortest way to say the most in the most accurate way.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.
"VU Pre-allocation" only makes sense to you because you already know it applies for arrival-rate executors. A new user won't know that fact and won't click on that menu entry at all, even if this is exactly the information they are looking for.
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.
Some more explanation is probably needed here 🤔 Something like this?
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.
My initial thought is that the info is good...but dense.
One thing, I see that I was just wrong to type per second, it should be iteration rate.
I also think a short bulleted intro should stay. Maybe not. This creates a progressive disclosure, where readers who quickly scan could get the minimal info they need. Opening with four paragraphs in a row might scare many off.
I've tried to synthesize your text and mine with:
977c45d
I wrote it sleepy, need to proofread, but let me know what you think.
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.
bullet points are fine, except when they force you to unnecessarily break up explanations so much that they stop making sense and are harder to understand, e.g. https://github.com/grafana/k6-docs/pull/974/files#r1069127202
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.
Maybe this can be moved in the
maxVUs
section? 🤔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 don't want to move the whole block down, because it's in an admonition and I think it's most respectful if we alert readers about things that can affect subscription use up front. But I'll move your addition to that section.
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 just realized this might be confusing in another way, "both
preAllocatedVUs
andmaxVUs
count against your subscription" may confuse users thatpreAllocatedVUs + maxVUs
will be counted against their subscription, while the reality is that just the bigger number between the two (i.e.max(preAllocatedVUs, maxVUs)
) will be counted.And, given that
maxVUs
will always be>= preAllocatedVUs
, and if you don't specifymaxVUs
explicitly, it's implicitly equal tomaxVUs
,maxVUs
is what will be counted against the subscriptionThere 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.
Maybe:
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.
Another problem - you mention
maxVUs
here, but we haven't mentioned maxVUs anywhere before in this document... This was one of the reasons for my longer explanation that you discarded, it had a paragraph with a cohesive explanation for bothpreAllocatedVUs
andmaxVUs
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 still kind of of the opinion that
maxVUs
shouldn't be mentioned anywhere :-). What I'll do is just make the first admonition only aboutpreAllocatedVUs
, and then a second admonition in themaxVUs
section. It's not very elegant, but it doesn't cram so much information together.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.
Again, if I could rewrite history, maxVUs probably would not exist 😅 But now that it exists, we should try to make it as clear as possible how it function and why it might not be a good idea to use.
An admonition only for
preAllocatedVUs
doesn't make sense, this is how cloud subscriptions normally work, i.e. no admonition needed just for it.And if we want to tuck
maxVUs
only at the end of the document, somewhat out of sight (which I don't necessarily mind), then we should only have an admonition there.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.
Here's how I've handled it: two admonitions. No points for subtlety, but I don't think anyone can say we're being sneaky.
EDIT: These are at the top and bottom of the page. In the GitHub UI it looks joined together.
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.
Nobody will read the text you have highlighted in the middle of that paragraph 😅 People will just see the two admonitions and get the very wrong impression that in the cloud we will charge them for
preAllocatedVUs + maxVUs
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.
Point taken. With 19784d4, it looks like this: