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

[ZEPPELIN-2606] Fix compilation with R interpreter enabled #2383

Closed
wants to merge 2 commits into from

Conversation

andreaTP
Copy link
Contributor

What is this PR for?

mvn -DskipTests clean package -Pr fails

What type of PR is it?

Bug Fix

What is the Jira issue?

[ZEPPELIN-2606]

How should this be tested?

mvn -DskipTests clean package -Pr

@zjffdu
Copy link
Contributor

zjffdu commented May 31, 2017

LGTM, but one concern is that why this bug is not found in CI, maybe we miss something in travis

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 1, 2017

yes a full compilation with all interpreters available should be included in CI build IMO ....

@khalidhuseynov
Copy link
Contributor

LGTM

@zjffdu
Copy link
Contributor

zjffdu commented Jun 1, 2017

I think r is missing in INTERPRETERS of .travis.yml

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 1, 2017

INTERPRETERS is surprisingly an exclusion list @zjffdu https://github.com/apache/zeppelin/blob/master/.travis.yml#L40

@zjffdu
Copy link
Contributor

zjffdu commented Jun 1, 2017

@andreaTP That's is intended. It would be converted to be inclusive.

MODULES="-pl $(echo .,zeppelin-interpreter,${INTERPRETERS} | sed 's/!//g')"

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 1, 2017

good point, so I added it to the list, I think it fits the scope of this PR (let see what travis says)

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 1, 2017

Since R is under spark we need to enable it differently AFAICS
now it is enabled at least in one test (where all other interpreters are compiled).
Travis is happy I close and re-open this.

@andreaTP andreaTP closed this Jun 1, 2017
@andreaTP andreaTP reopened this Jun 1, 2017
@jongyoul
Copy link
Member

jongyoul commented Jun 3, 2017

I have general question. Does -Pr works at the latest spark distribution?

@jongyoul
Copy link
Member

jongyoul commented Jun 6, 2017

ping

@necosta
Copy link
Contributor

necosta commented Jun 6, 2017

Q: Does -Pr works at the latest spark distribution?
A: Not sure. It works with 2.0.2. I haven't tried to compile it against 2.1.1. Any specific error you are getting?

@jongyoul
Copy link
Member

jongyoul commented Jun 6, 2017

Thanks, I just wonder if it works becase it was developed when spark was 1.5. One more question. Did you test -sparkR ? If you did, can you compare which one is proper for you? We had two different implementations and it's a bit hard to maintain it.

@necosta
Copy link
Contributor

necosta commented Jun 6, 2017

I'm no expert so don't trust my answer. I have both enabled and working, %r and %spark.r. The way I understand it, %r is just an interpreter to allow coding in R (as you would do in RStudio). And %spark.r is R + Spark, so you are able to interface with Spark in R as you would in Python or Scala. Which one to use?
depends on your use case I guess.

Copy link
Contributor

@necosta necosta left a comment

Choose a reason for hiding this comment

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

LGTM

@andreaTP
Copy link
Contributor Author

andreaTP commented Jun 6, 2017

do this need anything else to be merged?

@khalidhuseynov
Copy link
Contributor

will be merging into master if no more discussion

@asfgit asfgit closed this in 32d8afa Jun 8, 2017
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.

5 participants