ZEPPELIN-2685. Improvement on Interpreter class #2592

zjffdu · 2017-09-19T06:54:32Z

What is this PR for?

Follow up of #2577. Main changes on Interpreter

Add throw InterpreterException which is checked exception for the abstract methods of Interpreter, this would enforce the interpreter implementation to throw InterpreterException.
field name refactoring.
- property -> properties
- getProperty() --> getProperties()
Introduce launcher layer for interpreter launching. Currently we only use shell script to launch interpreter, but it could be any other service or component to launch interpreter, such as livy server , other 3rd party tools or even we may create a separate module for interpreter launcher
- abstract cass InterpreterLauncher
- For now, only 2 implementation: ShellScriptLauncher & SparkInterpreterLauncher. We could add method in class Interpreter to allow interpreter to specify its own launcher class, but it could be future work.

What type of PR is it?

[Improvement | Refactoring]

Todos

- Task

What is the Jira issue?

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

How should this be tested?

Unit test is covered. ShellScriptLauncherTest & SparkInterpreterLauncherTest

Screenshots (if appropriate)

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

zjffdu · 2017-09-21T11:13:11Z

@Leemoonsoo @jongyoul @felixcheung Could you help review it ? Thanks. It is followup of #2577

jongyoul · 2017-09-21T15:18:21Z

Launcher concept looks good to me.

felixcheung

two comments:

IMO it's useful to have a interpreter.sh script for interpreters. Even for Spark - I've seen a lot of cases where people are injecting dependencies, CLASS_PATH etc with it.
Looks like a few of these changes will break the interpreter interface - for custom interpreters not committed or released with Zeppelin, doesn't this break all of them? I have seen a few such interpreters having their own git repo and getting installed as jar or similar.

felixcheung · 2017-09-22T03:44:11Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java

+      try {
+        repl.cancel(getInterpreterContextWithoutRunner(null));
+      } catch (InterpreterException e) {
+        throw new RuntimeException(e);


why rewrap the exception here?

Because InterpreterException is checked exception. If I throw it, I have to change signature of the caller method, this would cause lots of code change. In order to keep the PR small, I just throw it as RuntimeException.

felixcheung · 2017-09-22T03:45:09Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java

+          try {
+            notebook.getInterpreterSettingManager().restart(setting.getId());
+          } catch (InterpreterException e) {
+            logger.warn("Fail to resetart interpreter: " + setting.getId(), e);


resetart -> restart

I think this should be an error...

felixcheung · 2017-09-22T03:48:47Z

...zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java

+    } else if (master.equals("yarn-cluster")) {
+      return "cluster";
+    } else if (master.startsWith("local")) {
+      return "client";


why is "local" == "client"?

isn't this never going to hit? isYarnMode is true before calling this method

there's only 2 valid value for spark.submit.deployMode (client or cluster). It is client for local mode IIUC.

felixcheung · 2017-09-22T03:50:38Z

...zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java

+    if (master == null) {
+      master = properties.getProperty("spark.master");
+      if (master == null) {
+        master = System.getenv("SPARK_HOME");


wait? master != SPARK_HOME?!?

my mistake. will fix it

felixcheung · 2017-09-22T03:54:58Z

...zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java

+    setupPropertiesForPySpark(sparkProperties);
+    setupPropertiesForSparkR(sparkProperties, properties.getProperty("SPARK_HOME"));
+    if (isYarnMode() && getDeployMode().equals("cluster")) {
+      env.put("SPARK_YARN_CLUSTER", "true");


thinking more about this, I think we should avoid Zeppelin specific env with a SPARK_ prefix, to avoid potential conflict

felixcheung · 2017-09-22T03:56:11Z

...elin-zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/ShellScriptLauncher.java

+    int connectTimeout =
+        zConf.getInt(ZeppelinConfiguration.ConfVars.ZEPPELIN_INTERPRETER_CONNECT_TIMEOUT);
+    if (option.isExistingProcess()) {
+      // TODO(zjffdu) remove the existing process approach seems no one is using this.


why is no one using existing process?

Remove this todo, as someone is using the existing process for restarting scenario [ZEPPELIN-2879]

felixcheung · 2017-09-22T03:58:38Z

spark/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java

@@ -353,7 +353,7 @@ private DepInterpreter getDepInterpreter() {
  public boolean isYarnMode() {
    String master = getProperty("master");
    if (master == null) {
-      master = getProperty().getProperty("spark.master", "local[*]");
+      master = getProperty("spark.master", "local[*]");
    }
    return master.startsWith("yarn");


will this be refactored to share code with SparkInterpreterLauncher.java?

They are in different module (spark, zeppelin-zengine). so I am afraid there's no chance to reuse code.

zjffdu · 2017-09-22T05:15:22Z

Looks like a few of these changes will break the interpreter interface - for custom interpreters not committed or released with Zeppelin, doesn't this break all of them? I have seen a few such interpreters having their own git repo and getting installed as jar or similar.

It would not break the existing interpreter. I introduce Launcher concept to allow customize interpreter to launch. But for now all the interpreter still use interpreter.sh to launch it even for spark. But I have a plan to create a specific script to launch spark, because there's lots of specific logic for spark interpreter.

Looks like a few of these changes will break the interpreter interface - for custom interpreters not committed or released with Zeppelin, doesn't this break all of them? I have seen a few such interpreters having their own git repo and getting installed as jar or similar.

Yes, it would break custom interpreter not committed with zeppelin. But it should be very easy to update the code.

felixcheung

public void open() throws InterpreterException will break the signature of any inheriting classes right?

zjffdu · 2017-10-10T05:51:19Z

@Leemoonsoo Do you have any concern about the compatibility break in this PR ?

Add throw InterpreterException which is checked exception for the abstract methods of Interpreter, this would enforce the interpreter implementation to throw InterpreterException.
field name refactoring.
** property -> properties
** getProperty() --> getProperties()

Leemoonsoo · 2017-10-10T16:19:24Z

@zjffdu I don't have much concern about compatibility break you have mentioned. Left few comments, please check.

Leemoonsoo · 2017-10-10T16:16:24Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java

@@ -626,6 +640,7 @@ public void clearNoteIdAndParaMap() {
    }
    return interpreters;
  }
+<<<<<<< 0c64d9ca676e48a749db9879fa3cebc06eb78b54


I think this is unexpected line

Leemoonsoo · 2017-10-10T16:18:20Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java

@@ -266,6 +272,14 @@ public InterpreterSetting(InterpreterSetting o) {
    this.conf = o.getConf();
  }

+  private void createLauncher() {
+    if (group.equals("spark")) {
+      this.launcher = new SparkInterpreterLauncher(this.conf);


@zjffdu Do you think it's possible to make this part more generic? For example, read class name of launcher from interpreter-setting.json and instantiate it using reflection. So any interpreter can define their own launcher without changing zeppelin core modules?

Yeah，I think about it before. But it may involve more work. Because I don't want to make interpreter jar mix with zeppelin server jar. Although I can use URLClassLoader, still feel a little risky to do it right now because Interpreter may not be instantiated correctly in zeppelin server process if it has some other dependencies, it requires interpreter do initialization in method open instead of its constructor.

zjffdu · 2017-10-13T06:08:03Z

Will merge it if no more discussion.

### What is this PR for? Fix build interpreters (apark and R) which broken after apache#2592 and apache#2596 ### What type of PR is it? [Hot Fix] ### How should this be tested? build interpreters ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: tinkoff-dwh <tinkoff.dwh@gmail.com> Closes apache#2630 from tinkoff-dwh/fix_r_interpretator and squashes the following commits: 0c9828b [tinkoff-dwh] [HOTFIX] fix build spark and R interpreters

### What is this PR for? This is for the bug hotfix introduced in #2592 . The issue is that new interpreter created can not run properly because the incorrect interpreter dir. Thanks tinkoff-dwh for reporting this issue. ### What type of PR is it? [ Hot Fix] ### Todos * [ ] - Task ### How should this be tested? * First time? Setup Travis CI as described on https://zeppelin.apache.org/contribution/contributions.html#continuous-integration * Strongly recommended: add automated unit tests for any new or changed behavior * Outline any manual steps to test the PR here. ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jeff Zhang <zjffdu@apache.org> Closes #2632 from zjffdu/HotFix_Interpreter and squashes the following commits: 2d7ab03 [Jeff Zhang] [HotFix] - Incorrect interpreter dir

zjffdu force-pushed the ZEPPELIN-2685 branch 8 times, most recently from 2421b85 to 3ce8544 Compare September 21, 2017 11:10

felixcheung reviewed Sep 22, 2017

View reviewed changes

felixcheung reviewed Sep 23, 2017

View reviewed changes

zjffdu force-pushed the ZEPPELIN-2685 branch from bc36d9e to b956863 Compare September 30, 2017 02:15

zjffdu force-pushed the ZEPPELIN-2685 branch 2 times, most recently from 967af4d to 1209873 Compare October 10, 2017 05:50

Leemoonsoo reviewed Oct 10, 2017

View reviewed changes

zjffdu force-pushed the ZEPPELIN-2685 branch from 1209873 to e0be073 Compare October 11, 2017 01:51

zjffdu added 2 commits October 12, 2017 11:16

ZEPPELIN-2685. Improvement on Interpreter class

e545cc3

address comments

17dc2f1

zjffdu force-pushed the ZEPPELIN-2685 branch from e0be073 to 17dc2f1 Compare October 12, 2017 03:17

asfgit closed this in 9812e26 Oct 14, 2017

tinkoff-dwh mentioned this pull request Oct 24, 2017

[HOTFIX] fix build spark and R interpreters #2630

Closed

zjffdu mentioned this pull request Oct 25, 2017

[HotFix] - Incorrect interpreter dir #2632

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZEPPELIN-2685. Improvement on Interpreter class #2592

ZEPPELIN-2685. Improvement on Interpreter class #2592

zjffdu commented Sep 19, 2017 •

edited

zjffdu commented Sep 21, 2017

jongyoul commented Sep 21, 2017

felixcheung left a comment

felixcheung Sep 22, 2017

zjffdu Sep 22, 2017

felixcheung Sep 22, 2017

felixcheung Sep 22, 2017

felixcheung Sep 22, 2017

felixcheung Sep 22, 2017

zjffdu Sep 22, 2017

felixcheung Sep 22, 2017

zjffdu Sep 22, 2017

felixcheung Sep 22, 2017

felixcheung Sep 22, 2017

zjffdu Sep 22, 2017 •

edited

felixcheung Sep 22, 2017

zjffdu Sep 22, 2017

zjffdu commented Sep 22, 2017 •

edited

felixcheung left a comment

zjffdu commented Oct 10, 2017

Leemoonsoo commented Oct 10, 2017

Leemoonsoo Oct 10, 2017

Leemoonsoo Oct 10, 2017

zjffdu Oct 11, 2017

zjffdu commented Oct 13, 2017

ZEPPELIN-2685. Improvement on Interpreter class #2592

ZEPPELIN-2685. Improvement on Interpreter class #2592

Conversation

zjffdu commented Sep 19, 2017 • edited

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

zjffdu commented Sep 21, 2017

jongyoul commented Sep 21, 2017

felixcheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjffdu Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjffdu commented Sep 22, 2017 • edited

felixcheung left a comment

Choose a reason for hiding this comment

zjffdu commented Oct 10, 2017

Leemoonsoo commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjffdu commented Oct 13, 2017

zjffdu commented Sep 19, 2017 •

edited

zjffdu Sep 22, 2017 •

edited

zjffdu commented Sep 22, 2017 •

edited