[SPARK-24653][tests] Avoid cross-job pollution in TestUtils / SpillListener. #21639
Conversation
This file contains hidden or 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
…stener. There is a narrow race in this code that is caused when the code being run in assertSpilled / assertNotSpilled runs more than a single job. SpillListener assumed that only a single job was run, and so would only block waiting for that single job to finish when `numSpilledStages` was called. But some tests (like SQL tests that call `checkAnswer`) run more than one job, and so that wait was basically a no-op. This could cause the next test to install a listener to receive events from the previous job. Which could cause test failures in certain cases. The change fixes that race, and also uninstalls listeners after the test runs, so they don't accumulate when the SparkContext is shared among multiple tests.
|
Test build #92312 has finished for PR 21639 at commit
|
Contributor
|
Seems the JIRA number is not related? |
jiangxb1987
reviewed
Jun 26, 2018
| body | ||
| assert(spillListener.numSpilledStages > 0, s"expected $identifier to spill, but did not") | ||
| withListener(sc, new SpillListener) { listener => | ||
| val ret = body |
Contributor
There was a problem hiding this comment.
Maybe I'm missing something obvious, but why shall we need the return value here?
Contributor
Author
There was a problem hiding this comment.
I saw the return type in the closure, but the method itself returns Unit, so all that can be cleaned up.
vanzin
changed the title
Contributor
Author
|
Oops, no idea how I got the wrong bug. |
vanzin
changed the title
|
Test build #92345 has finished for PR 21639 at commit
|
Contributor
|
lgtm |
Member
|
retest this please |
|
Test build #93063 has finished for PR 21639 at commit
|
Member
|
retest this please |
|
Test build #93091 has finished for PR 21639 at commit
|
Contributor
|
retest this please |
|
Test build #93185 has started for PR 21639 at commit |
srowen
approved these changes
Jul 31, 2018
| * this method will wait until all events posted to the listener bus are processed, and then | ||
| * remove the listener from the bus. | ||
| */ | ||
| def withListener[L <: SparkListener](sc: SparkContext, listener: L) (body: L => Unit): Unit = { |
|
Test build #4226 has finished for PR 21639 at commit
|
Member
|
retest this please |
|
Test build #93861 has finished for PR 21639 at commit
|
Member
|
Merged to master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
There is a narrow race in this code that is caused when the code being
run in assertSpilled / assertNotSpilled runs more than a single job.
SpillListener assumed that only a single job was run, and so would only
block waiting for that single job to finish when
numSpilledStageswascalled. But some tests (like SQL tests that call
checkAnswer) run morethan one job, and so that wait was basically a no-op.
This could cause the next test to install a listener to receive events
from the previous job. Which could cause test failures in certain cases.
The change fixes that race, and also uninstalls listeners after the
test runs, so they don't accumulate when the SparkContext is shared
among multiple tests.