[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter #1637

cloverhearts · 2016-11-15T11:55:48Z

What is this PR for?

Currently, the z.run command is restricted.
Only paragraphs in a single note can be executed.
I have modified this to allow you to freely execute paragraphs of other notes.
This PR provides the basis for the freeful use of Zeppelin's resources at each Interpreter implementation.

What type of PR is it?

Improvement, Feature

Todos

What is the Jira issue?

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

How should this be tested?

Currently under development.

run paragraph in same note

%spark
z.run("paragraphID")

run paragraph with external note

z.run("noteid", "paragraphid");

all note run

z.runNote("noteid");

Screenshots (if appropriate)

paragraph run
noterun

Questions:

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

…transaction.

# Conflicts: # spark/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java

Leemoonsoo

Thanks @cloverhearts for the contribution. I left some comments. Please take a look.

Leemoonsoo · 2016-11-17T01:27:18Z

zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/Interpreter.java

+   * get Remote Zeppelin Server Controller interface
+   */
+  @ZeppelinApi
+  public abstract RemoteWorksController getRemoteZeppelinServerController();


I think it's better pass RemoteWorksController through InterpreterContext from here, and avoid defining new methods in Interpreter class.

Leemoonsoo · 2016-11-17T01:42:31Z

...er/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java

@@ -43,6 +44,7 @@
  private int port = -1;
  private final String interpreterDir;
  private final String localRepoDir;
+  private RemoteWorksController remoteWorksController;


We need to avoid RemoteInterpreterManagedProcess holds reference to RemoteWorksController, because the object is not used in RemoteInterpreterManagedProcess.
Which can be done by adding new method in the RemoteInterpreterProcessListener interface.

Leemoonsoo · 2016-11-17T02:19:36Z

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java

@@ -118,6 +120,9 @@ public ZeppelinServer() throws Exception {

    notebook.addNotebookEventListener(heliumApplicationFactory);
    notebook.addNotebookEventListener(notebookWsServer.getNotebookInformationListener());
+
+    remoteWorksManager = new RemoteWorksManager(notebook);
+    replFactory.setRemoteController(remoteWorksManager.getInstance());


remoteWorksManager.getInstance() is invoked here only, not anywhere else.
That means we don't need singleton pattern here.

I deleted this class file.

Leemoonsoo · 2016-11-17T02:22:36Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/remoteworks/RemoteWorksManager.java

+    return RemoteWorksManager.instance;
+  }
+
+  private class NotebookJobManager implements RemoteWorksController {


What is purpose of having NotebookJobManager private class and RemoteWorksController interface?
I think all the methods in the NotebookJobManager can be moved to RemoteWorksManager, and RemoteWorksController interface can be removed.

I think that's simple and easier to understand.

This is actually a sign of my impatience.
I expected RemoteWorksController to exist in various cases in the future.
But this is not what we need now.
Your opinion is correct.
I will modify this.

Leemoonsoo · 2016-11-17T02:30:05Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

@@ -50,8 +50,10 @@ enum RemoteInterpreterEventType {
  OUTPUT_UPDATE = 9,
  ANGULAR_REGISTRY_PUSH = 10,
  APP_STATUS_UPDATE = 11,
+  REMOTE_ZEPPELIN_SERVER_CONTROL = 12


We can use more specific name for this. such as PARAGRAPH_RUN_CONTEXT.

Leemoonsoo · 2016-11-17T02:49:58Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

+  1: RemoteZeppelinServerControlEvent type,
+  2: string eventOwnerKey
+  3: string msg
+}


basically, we don't need enum RemoteZeppelinServerControlEvent, enum RemoteZeppelinServerResourceType, struct ZeppelinServerResourceParagraphRunner, struct ZeppelinServerResource, struct RemoteZeppelinServerController.

Please try use generic message data structure RemoteInterpreterEvent for sending event from interpreter process to zeppelin server process.

Thank you,
modified done. :)

Leemoonsoo · 2016-11-17T02:52:48Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

@@ -109,4 +140,6 @@ service RemoteInterpreterService {
  RemoteApplicationResult loadApplication(1: string applicationInstanceId, 2: string packageInfo, 3: string noteId, 4: string paragraphId);
  RemoteApplicationResult unloadApplication(1: string applicationInstanceId);
  RemoteApplicationResult runApplication(1: string applicationInstanceId);
+
+  void remoteZeppelinServerControlFeedback(1: RemoteZeppelinServerController response);


Please try use primitive types instead of RemoteZeppelinServerController. You can json serialize complex data into string like other rpc methods above.
And more specific name, such as 'sendRunContexts()`

# Conflicts: # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterEventPoller.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterEventType.java

# Conflicts: # scio/src/main/scala/org/apache/zeppelin/scio/ScioInterpreter.scala

# Conflicts: # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java # zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift # zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteAngularObjectTest.java # zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterOutputTestStream.java # zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterTest.java # zeppelin-interpreter/src/test/java/org/apache/zeppelin/resource/DistributedResourcePoolTest.java # zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/RemoteSchedulerTest.java # zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java

cloverhearts · 2016-11-18T06:07:42Z

@moon Thank you for your kind and good feedback.
sorry, my answer is late.
I confirmed your comments and checked everything for my code.
I think all of you are right.
I will fix most of the content.
Thank you.

cloverhearts · 2016-11-18T14:34:29Z

Change structure and remove remoteWorksManager
The event has been implemented using RemoteProcessEventListener.

cloverhearts · 2016-11-21T05:54:33Z

add feature for z.runNote("noteID")

cloverhearts · 2016-11-22T03:13:06Z

CI Error.

This is irrelevant to my edits.
It is also verified in other PRs.

Results :

Tests in error: 
  ParagraphActionsIT.testEditOnDoubleClick:443 » ElementNotVisible Element is no...
  ZeppelinIT.testAngularDisplay:142->AbstractZeppelinIT.waitForParagraph:70->AbstractZeppelinIT.pollingWait:96 » Timeout

cloverhearts · 2016-11-22T08:26:06Z

All work is done.

Leemoonsoo

Thanks @cloverhearts for addressing comments.

I left few more comments.
About some unnecessary changes around import blocks in many interpreter modules and for more consistency in thrift idl.

Leemoonsoo · 2016-11-22T21:03:44Z

alluxio/src/main/java/org/apache/zeppelin/alluxio/AlluxioInterpreter.java

-import org.apache.zeppelin.interpreter.InterpreterContext;
-import org.apache.zeppelin.interpreter.InterpreterPropertyBuilder;
-import org.apache.zeppelin.interpreter.InterpreterResult;
+import org.apache.zeppelin.interpreter.*;


Would you like to keep import block unchanged here (and the in the other interpreters)?

Leemoonsoo · 2016-11-22T21:05:16Z

pig/src/main/java/org/apache/zeppelin/pig/PigInterpreter.java

@@ -25,6 +25,7 @@
 import org.apache.zeppelin.interpreter.InterpreterContext;
 import org.apache.zeppelin.interpreter.InterpreterResult;
 import org.apache.zeppelin.interpreter.InterpreterResult.Code;
+import org.apache.zeppelin.interpreter.RemoteWorksController;


Could you keep PigInterpreter.java unchanged?

Leemoonsoo · 2016-11-22T21:05:41Z

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

@@ -114,6 +110,8 @@
  private SparkDependencyResolver dep;
  private String sparkUrl;

+  private RemoteWorksController remoteWorksController;


Could you keep SparkInterpreter.java unchanged?

Leemoonsoo · 2016-11-22T21:19:01Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

+struct RemoteZeppelinServerController {
+  2: string eventOwnerKey
+  3: string msg
+}


every message RemoteInterpreter send to ZeppelinServer using RemoteInterpreterEventClient is RemoteInterpreterEvent type. Shell we avoid defining new type - ZeppelinServerResourceParagraphRunner and RemoteZeppelinServerController - for the payload ?
Because all other event are sending their payload using RemoteInterpreterEvent instead of defining their own struct in thrift idl. So let's do it in consistent way.

Leemoonsoo · 2016-11-22T21:21:56Z

zeppelin-interpreter/src/main/thrift/RemoteInterpreterService.thrift

@@ -110,4 +122,6 @@ service RemoteInterpreterService {
  RemoteApplicationResult loadApplication(1: string applicationInstanceId, 2: string packageInfo, 3: string noteId, 4: string paragraphId);
  RemoteApplicationResult unloadApplication(1: string applicationInstanceId);
  RemoteApplicationResult runApplication(1: string applicationInstanceId);
+
+  void onReceivedResourceParagraphRunners(1: RemoteInterpreterEvent response);


This is message from ZeppelinServer to Interpreter process. Like other methods definitions, shell we not use RemoteInterpreterEvent and use primitive types instead? If you have a complex type payload, you can json serialize and send it as a string. (see angularObjectAdd at line 118)

cloverhearts · 2016-11-23T19:44:07Z

@Leemoonsoo
Thank you for your advice.
I have finished editing.
May I ask you for a review once again?

Leemoonsoo · 2016-11-24T18:18:57Z

Could you keep following files unchanged and resolve conflict with master?

bigquery/src/main/java/org/apache/zeppelin/bigquery/BigQueryInterpreter.java
cassandra/src/main/java/org/apache/zeppelin/cassandra/CassandraInterpreter.java
flink/src/main/java/org/apache/zeppelin/flink/FlinkInterpreter.java
ignite/src/main/java/org/apache/zeppelin/ignite/IgniteSqlInterpreter.java
jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
kylin/src/main/java/org/apache/zeppelin/kylin/KylinInterpreter.java
lens/src/main/java/org/apache/zeppelin/lens/LensInterpreter.java
markdown/src/main/java/org/apache/zeppelin/markdown/Markdown.java
pig/src/main/java/org/apache/zeppelin/pig/PigInterpreter.java
postgresql/src/main/java/org/apache/zeppelin/postgresql/PostgreSqlInterpreter.java
python/src/main/java/org/apache/zeppelin/python/PythonInterpreterPandasSql.java
scalding/src/main/java/org/apache/zeppelin/scalding/ScaldingInterpreter.java
spark/src/main/java/org/apache/zeppelin/spark/DepInterpreter.java
spark/src/main/java/org/apache/zeppelin/spark/PySparkInterpreter.java
spark/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java
spark/src/main/java/org/apache/zeppelin/spark/SparkSqlInterpreter.java
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/dev/DevInterpreter.java
zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/mock/MockInterpreterA.java
zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/mock/MockInterpreterB.java
zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/mock/MockInterpreterAngular.java
zeppelin-interpreter/src/test/java/org/apache/zeppelin/interpreter/remote/mock/MockInterpreterResourcePool.java
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java
zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter2.java

Except for that, LGTM

# Conflicts: # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterContext.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/InterpreterCompletion.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteApplicationResult.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterContext.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterEvent.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterResult.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterService.java

cloverhearts · 2016-11-26T09:44:13Z

One test case is experiencing a problem.
I will solve this problem.

cloverhearts · 2016-11-28T06:09:25Z

Okay, fixed all problem.
always ci green
i was change test case for zrun in angular object watch test.
@Leemoonsoo Could you please review for this pr?

Leemoonsoo · 2016-11-28T14:30:25Z

I still can see some unnecessary changes on the import code block in many files.
Could you manage them unchanged? I have listed files in previous comment.

cloverhearts · 2016-11-28T15:33:43Z

ci retry

cloverhearts · 2016-11-28T16:33:12Z

@Leemoonsoo Sorry, I think I misunderstood your answer. All edits are complete.
Thank you for your reply.

Leemoonsoo · 2016-11-28T16:39:28Z

@cloverhearts Thanks for the great contribution.
LGTM and merge to master if there're no more discussions.

cloverhearts added 7 commits November 11, 2016 12:14

code base workflow for remote zeppelin server control default thrift …

6e1f219

…transaction.

add remote works controller class and include interpreter factory

0570ae8

Implement eventForWait class

2523238

Implement getParagraphRunner transaction.

3d34f9e

remove debug console message.

3ed556c

Merge branch 'master' into workflow

afb9db7

# Conflicts: # spark/src/main/java/org/apache/zeppelin/spark/SparkInterpreter.java

fix sio support

5a7886f

cloverhearts changed the title ~~[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter~~ [ZEPPELIN-1665 : WIP] Z.run with external note executable and access resource for zeppelin in each interpreter Nov 15, 2016

cloverhearts added 2 commits November 15, 2016 22:55

support spark r

55e8704

support scald

3f75bd5

Leemoonsoo reviewed Nov 17, 2016

View reviewed changes

cloverhearts added 6 commits November 17, 2016 17:18

Merge branch 'master' into workflow

6fbe08a

# Conflicts: # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterEventPoller.java # zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/thrift/RemoteInterpreterEventType.java

fix thrift

2628a20

fix thrift interface

4b1ef08

fix sio support

c074f07

Merge branch 'workflow' into extends-zrun-remote-transaction

f11fed4

# Conflicts: # scio/src/main/scala/org/apache/zeppelin/scio/ScioInterpreter.scala

cloverhearts added 2 commits November 18, 2016 16:23

remote remoteworksController in interpreter.java

8cbe46c

Change structure and remove remoteWorksManager

9ab05af

cloverhearts added 2 commits November 21, 2016 11:27

Merge branch 'master' into extends-zrun-remote-transaction

f9661c8

Implement runNote and re implement run method

10c2a47

cloverhearts added 2 commits November 22, 2016 17:19

add test case for extends z.run and z.runNote

292319a

add document for extends z.run and z.runNote

342752d

cloverhearts changed the title ~~[ZEPPELIN-1665 : WIP] Z.run with external note executable and access resource for zeppelin in each interpreter~~ [ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter Nov 22, 2016

Leemoonsoo reviewed Nov 22, 2016

View reviewed changes

regenerate thrfit class

3862166

cloverhearts closed this Nov 25, 2016

cloverhearts reopened this Nov 25, 2016

cloverhearts closed this Nov 25, 2016

cloverhearts reopened this Nov 25, 2016

cloverhearts closed this Nov 25, 2016

cloverhearts reopened this Nov 25, 2016

cloverhearts added 2 commits November 26, 2016 12:11

last test case time check to print string

5a80a5a

fix TestCase

f2e3bcf

cloverhearts closed this Nov 26, 2016

cloverhearts reopened this Nov 26, 2016

cloverhearts closed this Nov 26, 2016

cloverhearts reopened this Nov 26, 2016

Merge branch 'master' into extends-zrun-remote-transaction

2a2c173

cloverhearts added 2 commits November 28, 2016 11:29

testcase change z.run(2, context) to z.run(2)

03a3a2b

Merge branch 'master' into extends-zrun-remote-transaction

113b475

restore unless changed and import

41fa9d7

cloverhearts closed this Nov 28, 2016

cloverhearts reopened this Nov 28, 2016

asfgit closed this in 9db840c Nov 29, 2016

Leemoonsoo mentioned this pull request Dec 11, 2016

ZEPPELIN-1692: Ability to access Spark jobs UI from the paragraph #1663

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter #1637

[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter #1637

cloverhearts commented Nov 15, 2016 •

edited

Leemoonsoo left a comment

Leemoonsoo Nov 17, 2016

Leemoonsoo Nov 17, 2016

Leemoonsoo Nov 17, 2016

cloverhearts Nov 18, 2016

Leemoonsoo Nov 17, 2016

cloverhearts Nov 18, 2016

Leemoonsoo Nov 17, 2016

Leemoonsoo Nov 17, 2016

cloverhearts Nov 18, 2016

Leemoonsoo Nov 17, 2016

cloverhearts commented Nov 18, 2016

cloverhearts commented Nov 18, 2016

cloverhearts commented Nov 21, 2016

cloverhearts commented Nov 22, 2016

cloverhearts commented Nov 22, 2016

Leemoonsoo left a comment

Leemoonsoo Nov 22, 2016

Leemoonsoo Nov 22, 2016

Leemoonsoo Nov 22, 2016

Leemoonsoo Nov 22, 2016

Leemoonsoo Nov 22, 2016

cloverhearts commented Nov 23, 2016

Leemoonsoo commented Nov 24, 2016

cloverhearts commented Nov 26, 2016

cloverhearts commented Nov 28, 2016

Leemoonsoo commented Nov 28, 2016

cloverhearts commented Nov 28, 2016

cloverhearts commented Nov 28, 2016

Leemoonsoo commented Nov 28, 2016

[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter #1637

[ZEPPELIN-1665] Z.run with external note executable and access resource for zeppelin in each interpreter #1637

Conversation

cloverhearts commented Nov 15, 2016 • 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:

Leemoonsoo 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

cloverhearts commented Nov 18, 2016

cloverhearts commented Nov 18, 2016

cloverhearts commented Nov 21, 2016

cloverhearts commented Nov 22, 2016

cloverhearts commented Nov 22, 2016

Leemoonsoo 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

cloverhearts commented Nov 23, 2016

Leemoonsoo commented Nov 24, 2016

cloverhearts commented Nov 26, 2016

cloverhearts commented Nov 28, 2016

Leemoonsoo commented Nov 28, 2016

cloverhearts commented Nov 28, 2016

cloverhearts commented Nov 28, 2016

Leemoonsoo commented Nov 28, 2016

cloverhearts commented Nov 15, 2016 •

edited