Skip to content

[SPARK-24535][SPARKR] fix tests on java check error #21666

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.

Sign up for GitHub

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

Jump to bottom
Closed
felixcheung wants to merge 5 commits into from
Closed

[SPARK-24535][SPARKR] fix tests on java check error #21666

felixcheung wants to merge 5 commits into from

Conversation

@felixcheung
Copy link
Member

@felixcheung felixcheung commented Jun 29, 2018 •
edited
Loading

What changes were proposed in this pull request?

change to skip tests if

  • couldn't determine java version

fix problem on windows

How was this patch tested?

unit test, manual, win-builder

@SparkQA
Copy link

SparkQA commented Jun 29, 2018

Test build #92456 has finished for PR 21666 at commit 35ab8a2.

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

@SparkQA
Copy link

SparkQA commented Jun 29, 2018

Test build #92457 has finished for PR 21666 at commit a4f9b1e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@felixcheung
Copy link
Member Author

felixcheung commented Jun 29, 2018

Jenkins, retest this please

@felixcheung
Copy link
Member Author

felixcheung commented Jun 29, 2018

@shivaram

@SparkQA
Copy link

SparkQA commented Jun 29, 2018

Test build #92461 has finished for PR 21666 at commit a4f9b1e.

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

shivaram
shivaram reviewed Jun 29, 2018
View reviewed changes
R/pkg/R/client.R Outdated

checkJavaVersion <- function() {
if (is_windows()) {
# See SPARK-24535
Copy link
Contributor

@shivaram shivaram Jun 29, 2018

Choose a reason for hiding this comment

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

But this just would end up skipping all tests on Windows ? I'm not sure if thats the right move here. Is there some way to run some tests on Windows ?

Copy link
Member Author

@felixcheung felixcheung Jun 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it would skip blindly yes - I guess we need to root cause why checkJavaVersion() fails on Windows/jdk 8 - otherwise sparkR.session() would just fail anyway and no way to continue to run/test anything.

Copy link
Member Author

@felixcheung felixcheung Jul 3, 2018

Choose a reason for hiding this comment

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

updated

@SparkQA
Copy link

SparkQA commented Jul 3, 2018

Test build #92556 has finished for PR 21666 at commit e1d1a64.

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

@SparkQA
Copy link

SparkQA commented Jul 3, 2018

Test build #92557 has finished for PR 21666 at commit 8d9ef83.

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

@jerryshao
Copy link
Contributor

jerryshao commented Jul 6, 2018

Any update on this issue @felixcheung , this blocks 2.3.2 release, just want to make sure if it is still in-progress.

@felixcheung
Copy link
Member Author

felixcheung commented Jul 6, 2018

sorry for the delay, I couldn't reply earlier - the fix was ready but I was hoping for a review.
anyway, I'm going to merge it now.

@felixcheung
Copy link
Member Author

felixcheung commented Jul 6, 2018

merged to master/2.3

asfgit pushed a commit that referenced this pull request Jul 6, 2018
@felixcheung
[SPARK-24535][SPARKR] fix tests on java check error
e5cc5f6 
## What changes were proposed in this pull request?

change to skip tests if
- couldn't determine java version

fix problem on windows

## How was this patch tested?

unit test, manual, win-builder

Author: Felix Cheung <felixcheung_m@hotmail.com>

Closes #21666 from felixcheung/rjavaskip.

(cherry picked from commit 141953f)
Signed-off-by: Felix Cheung <felixcheung@apache.org>
@asfgit asfgit closed this in 141953f Jul 6, 2018
@jerryshao
Copy link
Contributor

jerryshao commented Jul 6, 2018

Thanks @felixcheung .

@shivaram
Copy link
Contributor

shivaram commented Jul 6, 2018

Thanks @felixcheung and sorry for the delay in looking at this. I think the fix looks good. Overall it looks like we need to use system2 for the java version check as otherwise it runs inside a shell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@shivaram shivaram shivaram left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@felixcheung @SparkQA @jerryshao @shivaram