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-22990][Core] Fix method isFairScheduler in JobsTab and StagesTab #20186

Closed
wants to merge 1 commit into from

Conversation

gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented Jan 8, 2018

What changes were proposed in this pull request?

In current implementation, the function isFairScheduler is always false, since it is comparing String with SchedulingMode

It works with this patch:
wechatimg51

@cloud-fan
Copy link
Contributor

good catch! LGTM

@SparkQA
Copy link

SparkQA commented Jan 8, 2018

Test build #85801 has finished for PR 20186 at commit 10b5518.

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

@SparkQA
Copy link

SparkQA commented Jan 8, 2018

Test build #85800 has finished for PR 20186 at commit fb87aa6.

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

@gengliangwang
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Jan 8, 2018

Test build #85804 has finished for PR 20186 at commit 10b5518.

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

@vanzin
Copy link
Contributor

vanzin commented Jan 8, 2018

Is it easy to add a unit test in StagePageSuite? Also this is the kind of stuff the compiler should be complaining about...

@cloud-fan
Copy link
Contributor

I think it's fine. We usually don't test if a config can be correctly set as it's so trivial...

@cloud-fan
Copy link
Contributor

thanks, merging to master/2.3!

asfgit pushed a commit that referenced this pull request Jan 9, 2018
## What changes were proposed in this pull request?

In current implementation, the function `isFairScheduler` is always false, since it is comparing String with `SchedulingMode`

Author: Wang Gengliang <[email protected]>

Closes #20186 from gengliangwang/isFairScheduler.

(cherry picked from commit 849043c)
Signed-off-by: Wenchen Fan <[email protected]>
@asfgit asfgit closed this in 849043c Jan 9, 2018
@gengliangwang
Copy link
Member Author

gengliangwang commented Jan 9, 2018

I just did a quick test with code changes, it works. Attach the screenshot in PR description.

@gengliangwang gengliangwang deleted the isFairScheduler branch January 9, 2018 02:50
@vanzin
Copy link
Contributor

vanzin commented Jan 9, 2018

I think it's fine. We usually don't test if a config can be correctly set as it's so trivial...

The point is to test that the code using that config works as expected, not that it can be set. And as this PR proves, code can be wrong even in "trivial" cases.

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.

4 participants