Clean up and optimize http.batch(), fix batchPerHost #1259
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
This is necessary for us to refactor the http.batch() method, mostly because of dop251/goja#84 ( specifically, for https://github.com/loadimpact/k6/blob/a2e1c6f4e6207d69461a1b0f24c3aa2c72233101/js/modules/k6/http/response.go#L36-L38 )
Codecov Report
@@ Coverage Diff @@
## master #1259 +/- ##
=========================================
+ Coverage 75.31% 75.4% +0.08%
=========================================
Files 149 150 +1
Lines 10844 10883 +39
=========================================
+ Hits 8167 8206 +39
Misses 2211 2211
Partials 466 466
Continue to review full report at Codecov.
|
mstoykov
reviewed
Dec 4, 2019
|
I think a slight further optimization of this could be using a slice+atomic instead of a buffered channel, and using a double pointer instead of copying the response data |
na--
force-pushed
the
make-batch-great-again
branch
from
45fef98
to
016152d
Compare
cuonglm
suggested changes
Dec 4, 2019
imiric
reviewed
Dec 4, 2019
imiric
reviewed
Dec 4, 2019
imiric
reviewed
Dec 4, 2019
mstoykov
reviewed
Dec 6, 2019
na--
changed the title
na--
changed the title
This adds a minor breaking change - if some of the requests in the http.batch() fail, now the first error would be returned instead of the last one.
na--
force-pushed
the
make-batch-great-again
branch
from
e00691e
to
079f63c
Compare
na--
force-pushed
the
make-batch-great-again
branch
from
079f63c
to
f950d67
Compare
cuonglm
approved these changes
Dec 6, 2019
imiric
approved these changes
Dec 6, 2019
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.
This removes unnecessary goroutine spawning, synchronization, type conversion, etc. I had to update goja because of dop251/goja#84, since I needed to refactor this:https://github.com/loadimpact/k6/blob/a2e1c6f4e6207d69461a1b0f24c3aa2c72233101/js/modules/k6/http/response.go#L36-L38
I also reduced the default
batchPerHostvalue from 20 to 6, so k6 would be more in-line with browsers. Also, thanks to @mstoykov and #1264, the default value ofbatchPerHostnow actually works.Closes #767 and #967 (because of the goja update)