revert [SPARK-10030] Use tags to control which tests to run depending on changes being tested #20310
Conversation
|
Test build #86321 has finished for PR 20310 at commit
|
|
Are you sure that we want to blanket revert this entire patch? Is there a more surgical short-term fix we can make in Also, is this the first time recently that we've failed the YARN integration tests? How much time do they add? The trade off here seems to be between slightly slower after-the-fact detection of a test failure / build break due to YARN vs. faster tests for the majority of PRs that don't touch YARN code. I think we've had one or two such breaks in the 2+ years that we've been using these test tags, so I'd also be fine with postponing this change if you agree that it's unlikely that we're going to have many such failures here. If the motivation is that it's hard to test the fix for such build breaks (because the failing test wouldn't be exercised in the PR builder) then I think we might already have a solution via special tags placed into the PR title (I think |
|
We want a way to test yarn for the new fix: #20297 I checked |
| @TagAnnotation | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.METHOD, ElementType.TYPE}) | ||
| public @interface DockerTest { } |
There was a problem hiding this comment.
We don't use this docker tag in modules.py, does it mean we never run docker tests in PR builder?
There was a problem hiding this comment.
if it is, we should preserve this behavior.
There was a problem hiding this comment.
If you search through the commit history, I'm pretty sure that we originally tried running those DockerTests on RISELAB Jenkins but ran into problems with the Docker Daemon becoming unstable under heavy build load. This should be fixed in the newer-generation Ubuntu build workers, but we haven't quite finished migrating the PRBs onto those.
Given this, my hunch is that those tests aren't running anywhere right now, which isn't great. I think they're primarily used for testing JDBC data source SQL dialect mappings. It's been a year or more since I last looked into this, though, so I might be misremembering.
|
Test build #86325 has finished for PR 20310 at commit
|
…on changes being tested
|
Test build #86333 has finished for PR 20310 at commit
|
In that case we're ok because that fix touches YARN code, so YARN tests are running. |
|
To answer one of Josh's question, from the link above, it seems the YARN integration tests currently add ~5m to the PRB time. That doesn't sound too horrible, but would potentially make current timeout issues worse since we seem to be brushing against the existing limit already. |
What changes were proposed in this pull request?
This PR reverts #8775
The problem is that, when we change the code in Spark core, we may break yarn test, so we should run yarn test even we didn't change code in the yarn module. We broke the build because of this issue in #20223
How was this patch tested?
N/A