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-4323] Kotlin support for Spark interpreter #3440

Closed
wants to merge 121 commits into from

Conversation

dkaznacheev
Copy link
Contributor

@dkaznacheev dkaznacheev commented Sep 6, 2019

What is this PR for?

This PR aims to add Kotlin language support to Apache Zeppelin as a standalone interpreter as well as a part of Spark interpreter group.
For added features and details, see Jira issue: https://issues.apache.org/jira/browse/ZEPPELIN-4323

Feel free to ask any questions, I will be happy to answer!

What type of PR is it?

Feature

Todos

  • - Add support for Spark versions older than 2.4

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-4323

How should this be tested?

  • Unit tests are added to standalone Kotlin interpreter and to Spark interpreter.
  • Manually test by building Zepppelin under profile -Pspark2.4 and using %kotlin in standalone interpreter or %spark.kotlin in Spark interpreter

Screenshots (if appropriate)

Screenshot 2019-09-06 at 14 58 00

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes, and documentation is updated for Kotlin and Spark


private List<String> sparkClasspath() {
String sparkJars = System.getProperty("spark.jars");
Pattern isKotlinJar = Pattern.compile("/kotlin-(runtime|stdlib|compiler|reflect)(-.*)?\\.jar");
Copy link

Choose a reason for hiding this comment

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

May be this pattern simplified to /kotlin-[a-z]*(-.*)?\\.jar? I suppose we don't want to change this regex every time new jar is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

private List<String> sparkClasspath() {
String sparkJars = System.getProperty("spark.jars");
Copy link

Choose a reason for hiding this comment

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

Shouldn't this method be static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -128,7 +128,8 @@ static void atomicWriteToFile(String content, File file) throws IOException {
}
try {
file.getParentFile().mkdirs();
Files.move(tempFile.toPath(), destinationFilePath, StandardCopyOption.ATOMIC_MOVE);
Files.move(tempFile.toPath(), destinationFilePath,
StandardCopyOption.REPLACE_EXISTING); //StandardCopyOption.ATOMIC_MOVE);
Copy link

@ileasile ileasile Nov 8, 2019

Choose a reason for hiding this comment

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

Why has this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Atomic move caused errors on Linux, I suggest asking khud about that

@zjffdu
Copy link
Contributor

zjffdu commented Nov 13, 2019

@dkaznacheev Could you resolve the conflict ?

@ileasile
Copy link

@zjffdu Conflict was resolved

@zjffdu
Copy link
Contributor

zjffdu commented Nov 13, 2019

@ileasile @dkaznacheev Could any of you paste the travis CI link ?

@ileasile
Copy link

@zjffdu
Copy link
Contributor

zjffdu commented Nov 14, 2019

Thanks @ileasile Could you rerun the failed test ?

@zjffdu
Copy link
Contributor

zjffdu commented Nov 18, 2019

Thanks @ileasile the PR LGTM except on minor thing.

Kotlin unit test is ignored for now, you need to add org.apache.zeppelin.kotlin.* in one of the build, such as this one for spark24 https://github.com/apache/zeppelin/blob/master/.travis.yml#L115

We do the same thing for python interpreter, https://github.com/apache/zeppelin/blob/master/.travis.yml#L123

@ileasile
Copy link

@zjffdu This issue is fixed, new build is here: https://travis-ci.org/ileasile/zeppelin

@zjffdu
Copy link
Contributor

zjffdu commented Nov 19, 2019

LGTM, will merge if no more comments

@ileasile
Copy link

@zjffdu Yes, this may be already merged

@asfgit asfgit closed this in ee2613f Nov 19, 2019
@pan3793
Copy link
Member

pan3793 commented Sep 11, 2023

@dkaznacheev @ileasile the Koltin version is pretty old and depends on unstable API, do you guys have a plan to upgrade it to keep things up-to-date?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants