Remove the core.Engine 馃帀
#2815
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 was referenced Dec 9, 2022
codebien
reviewed
Dec 21, 2022
Comment on lines
+167
to
+176
| // We call waitOutputsFlushed() below because the threshold calculations | ||
| // need all of the metrics to be sent to the MetricsEngine before we can | ||
| // calculate them one last time. We need the threshold calculated here, | ||
| // since they may change the run status for the outputs. |
There was a problem hiding this comment.
Instead, to do this by the output manager, could we do an explicit call to the ingester? I think it would simplify a bit the logic.
There was a problem hiding this comment.
hmm, maybe, not sure it would be simpler though 馃 here in the defer statements, we just have the following sequence after the test run finishes:
- close the
sampleschannel - wait for all outputs to finish flushing their metrics
- finalize the thresholds (unless
--no-thresholdswas used) - stop all outputs
to me, this makes sense even if we completely ignore thresholds 馃 and I am not sure how we can simplify it, when the piping of the metrics also happens by the output.Manager 馃
There was a problem hiding this comment.
Not blocking point: I think it could be helpful to add these 4 points to the comment so we have one central place where we can read the flow without going across the entire code of the Run method. I think it could be helpful when we will work on #2430.
There was a problem hiding this comment.
馃憤, but let's add this in the PR that converts the few remaining tests for the old Engine, I'll merge this one shortly
codebien
requested changes
Dec 23, 2022
codebien
reviewed
Dec 23, 2022
na--
force-pushed
the
prune-the-engine
branch
from
195c2ab
to
818cee6
Compare
na--
force-pushed
the
remove-the-engine
branch
from
3dbeb3b
to
8324c42
Compare
Codecov Report
@@ Coverage Diff @@
## master #2815 +/- ##
==========================================
+ Coverage 76.81% 76.84% +0.03%
==========================================
Files 225 222 -3
Lines 16867 16846 -21
==========================================
- Hits 12956 12945 -11
+ Misses 3078 3065 -13
- Partials 833 836 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
na--
force-pushed
the
prune-the-engine
branch
from
818cee6
to
d24bcc2
Compare
na--
force-pushed
the
remove-the-engine
branch
3 times, most recently
from
4dd18e5
to
2e4f071
Compare
na--
force-pushed
the
prune-the-engine
branch
3 times, most recently
from
30908a1
to
cb17842
Compare
na--
force-pushed
the
remove-the-engine
branch
2 times, most recently
from
3c00d58
to
6f6b105
Compare
Merged
na--
force-pushed
the
prune-the-engine
branch
2 times, most recently
from
c234955
to
d0eb9d3
Compare
na--
force-pushed
the
remove-the-engine
branch
from
9fd527b
to
63ae6b0
Compare
na--
force-pushed
the
remove-the-engine
branch
2 times, most recently
from
16c82bc
to
ee62441
Compare
na--
added a commit
that referenced
this pull request
Jan 30, 2023
This is the first commit from #2815, plus an extra test that checks the exit code when aborting a `k6 run --paused` test with Ctrl+C. Unfortunately, it turned out that 19dd505 from #2813 introduced a regression. Interrupting the k6 process with a signal should result in a `105` exit code (`ExternalAbort`), but with that commit k6 exited with `103` if k6 was `--paused`. The good news is that the fix was already done in the first commit of #2815, which was entirely self-sufficient and can be moved to this separate PR. As a bonus, this makes reviewing everything even easier... So, some details about the commit itself... In short, there is no need to have 2 separate `Run()` and `Init()` methods in the `execution.Scheduler`. Instead, Run() can start the VU initialization itself. As a bonus, this immediately makes the error handling around init errors much more in line with other error handling, allowing us to resolve the bug, but also to respect `--linger` and to try and execute `handleSummary()` if there were problems. Except the first init that is used to get the exported options, of course, that one is still special.
na--
force-pushed
the
remove-the-engine
branch
from
ee62441
to
943b1e7
Compare
mstoykov
reviewed
Feb 1, 2023
na--
force-pushed
the
remove-the-engine
branch
from
e87ee32
to
27b1a4d
Compare
This reverts a big part of #2885 because with all of the logic in Run(), k6 didn't know if it needed to wait when --linger was specified. In general, if a script error or test.abort() occurs during init, --linger should not apply and k6 should exit immediately. But if test.abort() is called during the test run itself, or the test was stopped in some other way besides Ctrl+C, --linger means that k6 should not exit immediately after stopping the test.
mstoykov
approved these changes
Feb 2, 2023
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 PR builds on top of #2813 and closes #1889, finally 馃帀 馃帄 It also leaves us in a much better position to implement #1342, #140, #2804, #2430, and a myriad of other issues 馃帀
Some of the less important tests still need to be ported, so it it still technically a draft PR that's not quite ready to merge as it is. Still, given the fact that the new integration tests all work fine, I am fairly confident there aren't any major problems 馃 I also don't intend to make any big changes to the code here, so it's more than ready for review.