Skip to content

SPARK-1143: refactor TaskSchedulerImplSuite - #339

Closed
CodingCat wants to merge 7 commits into
apache:masterfrom
CodingCat:SPARK-1143
Closed

CodingCat wants to merge 7 commits into
apache:masterfrom
CodingCat:SPARK-1143

Conversation

@CodingCat

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SPARK-1143

The current TaskSchedulerImplSuite actually bypasses the scheduling code (mainly resourceOffer())

In this patch, I removed the original dummy ResourceOffer and made the test case call taskScheduler.resourceOffer() directly,

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@CodingCat CodingCat changed the title SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffer [WIP]SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffer Apr 6, 2014
@CodingCat CodingCat changed the title [WIP]SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffer SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffer Apr 6, 2014
@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13825/

@CodingCat CodingCat changed the title SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffer SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffers Apr 6, 2014
@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13826/

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. All automated tests passed.

@AmplabJenkins

Copy link
Copy Markdown

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13828/

@CodingCat

Copy link
Copy Markdown
Contributor Author

the previous failures are due to timeout and failed to fetch remote repo,

anyone wants to review this?

andrewor14 pushed a commit to andrewor14/spark that referenced this pull request Apr 7, 2014
Conf improvements

There are two new features.

1. Allow users to set arbitrary akka configurations via spark conf.

2. Allow configuration to be printed in logs for diagnosis.
@CodingCat

Copy link
Copy Markdown
Contributor Author

Hi, @kayousterhout , mind reviewing this?

@kayousterhout

Copy link
Copy Markdown
Contributor

I won't have time to review this for a week or so

@CodingCat

Copy link
Copy Markdown
Contributor Author

sure, take your time

I notified you just because you're the original reporter of the issue and I want to ensure I understand the issue correctly

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. All automated tests passed.

@AmplabJenkins

Copy link
Copy Markdown

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14066/

@CodingCat

Copy link
Copy Markdown
Contributor Author

rebased~

@kayousterhout

Copy link
Copy Markdown
Contributor

The problem I was describing in the JIRA is that all but the last 2 tests in this suite are designed to test the pool / fairness mechanisms (i.e., that the output of rootPool.getSortedTaskSetQueue() is correct), not the scheduler code. I think the right solution here is to move these tests into a separate test suite specifically for the pool stuff -- not to make these tests use the scheduler (since using the scheduler obfuscates where the actually error would be if the tests failed).

@CodingCat

Copy link
Copy Markdown
Contributor Author

Hi, @kayousterhout, you mean just split the test cases and move some of them to another file?

I can do that...but do you think the test cases specific to the TaskSchedulerImpl class are also missed? (except the last two)

I think the implementation in this patch is equivalent to the original one, as we assume the locality is Any and the assert statements are actually testing the results of rootPool.getSortedTaskSetQueue(),

I think for fully test the critical code path of TaskSchedulerImpl, we need some test cases on the locality?

@kayousterhout

Copy link
Copy Markdown
Contributor

We definitely don't have sufficient testing for TaskSchedulerImpl right now
(since the only tests that test that at all are the last two in this file),
so at some point adding more tests for that would be good too.

On Sun, Apr 13, 2014 at 2:57 PM, Nan Zhu [email protected] wrote:

Hi, @kayousterhout https://github.com/kayousterhout, you mean just
split the test cases and move some of them to another file?

I can do that...but do you think the test cases specific to the
TaskSchedulerImpl class are also missed?

I think the implementation in this patch is equivalent to the original
one, as we assume the locality is Any and the assert statements are
actually testing the results of rootPool.getSortedTaskSetQueue(),

I think for fully test the critical code path of TaskSchedulerImpl, we
need some test cases on the locality?

Reply to this email directly or view it on GitHubhttps://github.com//pull/339#issuecomment-40321243
.

@CodingCat CodingCat changed the title SPARK-1143: refactor TaskSchedulerImplSuite using taskScheduler.resourceOffers SPARK-1143: refactor TaskSchedulerImplSuite May 13, 2014
@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

QA tests have finished for PR 339 at commit 4341383.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20701/

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

QA tests have started for PR 339 at commit 0df7499.

  • This patch merges cleanly.

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

QA tests have finished for PR 339 at commit 0df7499.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20702/

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

QA tests have started for PR 339 at commit 50e2463.

  • This patch merges cleanly.

@nchammas

Copy link
Copy Markdown
Contributor

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20702/

I don't know where this message is coming from, and I don't see any recent changes to the dev/run-tests-jenkins script to explain it.

@pwendell?

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

QA tests have finished for PR 339 at commit 50e2463.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Sep 23, 2014

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20705/

@nchammas

nchammas commented Jan 4, 2015

Copy link
Copy Markdown
Contributor

@CodingCat @kayousterhout What's the status of this PR? It hasn't been updated in several months.

@andrewor14

Copy link
Copy Markdown
Contributor

@kayousterhout does this PR reflect what you had in mind in your description for SPARK-1143

@CodingCat

Copy link
Copy Markdown
Contributor Author

@andrewor14 I need to add a design doc then?

@kayousterhout

Copy link
Copy Markdown
Contributor

@CodingCat @andrewor14 This isn't quite what I had in mind; I'm going to submit a (simpler) PR for this in the next few hours. I think this one is more complicated than necessary.

@andrewor14

Copy link
Copy Markdown
Contributor

No need for design doc; I thought for a second you were refactoring TaskSchedulerImpl itself. My mistake.

@andrewor14

Copy link
Copy Markdown
Contributor

Ok, since @kayousterhout is planning on submitting a simpler version of this, @CodingCat would you mind closing this PR?

@CodingCat

Copy link
Copy Markdown
Contributor Author

sure

@CodingCat CodingCat closed this Jan 8, 2015
kayousterhout added a commit to kayousterhout/spark-1 that referenced this pull request Jan 9, 2015
The current TaskSchedulerImplSuite includes some tests that are
actually for the TaskSchedulerImpl, but many of the tests avoid using
the TaskSchedulerImpl entirely, and actually test the pool and scheduling
algorithm mechanisms. This commit separates the pool/scheduling algorithm
tests into their own suite, simplifies those tests, and attempts to
more helpfully comment the fair scheduler test.

The pull request replaces apache#339.
asfgit pushed a commit that referenced this pull request Jan 9, 2015
The current TaskSchedulerImplSuite includes some tests that are
actually for the TaskSchedulerImpl, but the remainder of the tests avoid using
the TaskSchedulerImpl entirely, and actually test the pool and scheduling
algorithm mechanisms. This commit separates the pool/scheduling algorithm
tests into their own suite, and also simplifies those tests.

The pull request replaces #339.

Author: Kay Ousterhout <[email protected]>

Closes #3967 from kayousterhout/SPARK-1143 and squashes the following commits:

8a898c4 [Kay Ousterhout] [SPARK-1143] Separate pool tests into their own suite.
bzhaoopenstack pushed a commit to bzhaoopenstack/spark that referenced this pull request Sep 11, 2019
Fix if issue in keyword contain case
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Sep 24, 2026
### What changes were proposed in this pull request?

Step 6a of task 87 (`sql/varka/plans/PLAN_TASK_87.md`, section 6): `VarkaMethodSizeBenchmark` gains its second arm and its committed files are regenerated with both forms in one table per rung, banded over ten runs. Stacked on apache#339 (step 5); the diff here is the last two commits once that merges.

The two forms are named explicitly so the labels survive the default changing: `single epilogue`, the legacy emission (`methodByteBudget` 0), and `epilogue per group`, the emission under the budget at `HugeMethodLimit` - a group's methods set up only their group (apache#337), the epilogue is one method per group (apache#338), and the class is measured and regrouped (apache#339). `VarkaEmitBudget` is public so a benchmark outside the emitter's package can name the limit. The results, in M rows/s at 512 bits on the masked arm (the arm that reaches the single epilogue's crossing at thirteen outputs):

| outputs | even batches | ragged batches |
| ---: | :--- | :--- |
| 4 | 441.9 -> 434.1 | 431.2 -> 442.8 |
| 12 | 134.0 -> 143.1 | 135.2 -> 142.3 |
| 13 | 120.9 -> 132.9 | 40.7 -> **131.7** |
| 16 | 94.4 -> 106.3 | 32.9 -> 104.9 |
| 60 | 24.1 -> 29.4 | 8.7 -> 28.9 |

The cliff is gone: on ragged batches the per-group form reads within a few percent of its own even-batch rate at every rung, where the single form had lost three quarters of its rate from thirteen outputs. And from twelve outputs the per-group form is faster on even batches too, by 7% at twelve and 22% at sixty, which no prediction had: each loop method now sets up only its group's segments and literals, and that outweighs the driver's extra epilogue calls. The 128-bit file reads the same way (13 outputs ragged: 24.9 -> 44.0). Section 9.5 of the plan scores the registered predictions: 3 held and understated the gain, 4 fell on the fast side, 5 is superseded by the design; the default's rule - nowhere worse beyond its band on the masked arm at any rung, length or width, and on the null-free arm up to twelve outputs - is met, and the flip is step 6b.

One lead for task 189: in this fork the single form's null-free rows at 128 bits from twelve outputs sit in the deoptimization cycle (0.3 to 0.7 M rows/s ragged) and the per-group form's do not (40 to 56). One fork proves nothing about a bimodal effect; the plan records it as a lead, not a property.

### Why are the changes needed?

The plan's rule for flipping the default is a measurement, and this is it: both forms in one file, at both widths, with the band that says which differences mean anything.

### Does this PR introduce _any_ user-facing change?

No. The default is unchanged in this PR.

### How was this patch tested?

The benchmark is the test: `dev/varka_bench_regen.sh catalyst VarkaMethodSizeBenchmark` (pinned, load 0.57 at start, canary clean, invariants ok at both widths) and `dev/varka_bench_repeat.sh catalyst VarkaMethodSizeBenchmark 10 --band`. `dev/varka_quote_check.py` passes for every number quoted in the plan. The suites do not change.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Fable 5.1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants