Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-26082][MESOS][FOLLOWUP][BRANCH-2.3] Add UT on fetcher cache option on MesosClusterScheduler #23754

Closed

Conversation

HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Feb 9, 2019

What changes were proposed in this pull request?

This patch adds UT on testing SPARK-26082 to avoid regression. While #23743 reduces the possibility to make a similar mistake, the needed lines of code for adding tests are not that huge, so I guess it might be worth to add them.

How was this patch tested?

Newly added UTs. Test "supports setting fetcher cache" fails when #23734 is not applied and succeeds when #23734 is applied.

…sClusterScheduler

This patch adds UT on testing SPARK-26082 to avoid regression. While apache#23743 reduces the possibility to make a similar mistake, the needed lines of code for adding tests are not that huge, so I guess it might be worth to add them.

Newly added UTs. Test "supports setting fetcher cache" fails when apache#23743 is not applied and succeeds when apache#23743 is applied.
@SparkQA
Copy link

SparkQA commented Feb 9, 2019

Test build #102122 has finished for PR 23754 at commit 04abcd9.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-26082][MESOS][FOLLOWUP][BRANCH-2.3] Add UT on fetcher cache option on MesosClusterScheduler [SPARK-26082][MESOS][BRANCH-2.3] Add UT on fetcher cache option on MesosClusterScheduler Feb 9, 2019
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-26082][MESOS][BRANCH-2.3] Add UT on fetcher cache option on MesosClusterScheduler [SPARK-26082][MESOS][FOLLOWUP][BRANCH-2.3] Add UT on fetcher cache option on MesosClusterScheduler Feb 9, 2019
@dongjoon-hyun
Copy link
Member

Please fix the incorrect PR description.

Newly added UTs. Test "supports setting fetcher cache" fails when #23743 is not applied and succeeds when #23743 is applied.

@HeartSaVioR
Copy link
Contributor Author

My bad. I fixed the number (number being swapped at that time).

@@ -254,6 +254,54 @@ class MesosClusterSchedulerSuite extends SparkFunSuite with LocalSparkContext wi
assert(networkInfos.get(0).getLabels.getLabels(1).getValue == "val2")
}

test("supports setting fetcher cache") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add SPARK-26082 at the test case name.

assert(uris.asScala.forall(_.getCache))
}

test("supports disabling fetcher cache") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add SPARK-26082 at the test case name.

@dongjoon-hyun
Copy link
Member

I know this is the same with the patch on branch-2.4. But, I want to make it sure that this patch doesn't break the build and pass all test.

@SparkQA
Copy link

SparkQA commented Feb 10, 2019

Test build #102164 has finished for PR 23754 at commit 705b19b.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Merged to branch-2.3.

dongjoon-hyun pushed a commit that referenced this pull request Feb 10, 2019
…tion on MesosClusterScheduler

## What changes were proposed in this pull request?

This patch adds UT on testing SPARK-26082 to avoid regression. While #23743 reduces the possibility to make a similar mistake, the needed lines of code for adding tests are not that huge, so I guess it might be worth to add them.

## How was this patch tested?

Newly added UTs. Test "supports setting fetcher cache" fails when #23734 is not applied and succeeds when #23734 is applied.

Closes #23754 from HeartSaVioR/SPARK-26082-branch-2.3.

Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member

Thank you, @HeartSaVioR .

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.

3 participants