[ZEPPELIN-940] Allow zeppelin server to connect to already executing Remote Interpreter by SachinJanani · Pull Request #955 · apache/zeppelin

SachinJanani · 2016-06-03T13:18:03Z

What is this PR for?

Currenlty zeppelin server starts interpreter on localhost and with random port.The purpose of this pull request is to allow zeppelin server to connect to remotely executing zeppelin interpreter that user might have started in his service.This feature will be further helpful while cluster manager is to be implemented.(https://cwiki.apache.org/confluence/display/ZEPPELIN/Cluster+Manager+Proposal)

What type of PR is it?

Improvement

Todos

[ ] -Add documentation

What is the Jira issue?

[ZEPPELIN-940] https://issues.apache.org/jira/browse/ZEPPELIN-940

How should this be tested?

Added Junit test in RemoteInterpreterProcessTest and it passes

Screenshots (if appropriate)

Questions:

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

Latest update

…Remote Interpreter

…ppelin

Leemoonsoo · 2016-06-04T02:06:16Z

Thanks @SachinJanani for the contribution.

Ideally configure host and port can be moved from Property to InterpreterOption class.

That'll requires some frontend work, too. It's basically add UI component for host, port under 'Option' section, in interpreter setting page (e.g. https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-web/src/app/interpreter/interpreter-create/interpreter-create.html#L38)

@SachinJanani Could you manage to do it? Let me know if you need any help!

SachinJanani · 2016-06-05T09:49:22Z

@Leemoonsoo You mean that there should be one more option under option dropdown something like "isexecuting" and when this option is selected we have to show 2 properties Host and port is separate section?

Leemoonsoo · 2016-06-05T19:59:18Z

@SachinJanani Exactly!

…n is selected

SachinJanani · 2016-06-06T08:37:03Z

@Leemoonsoo As suggested i have added UI component for the host and port in the interpreter settings.Please see the attached image of how it will look.

…Remote Interpreter

…n is selected

…ppelin

Leemoonsoo · 2016-06-06T15:37:33Z

@SachinJanani Thanks for the update!

In 'option' section, 'shard | scope | isolated' dropdown is more for how interpreter instances/processes are mapped to notebooks. 'executing' is little bit different from them i think. So how about separate from dropdown list? I think something like a check box could work to toggle 'executing'.

And what do you think about use little bit more specific name for label and variable name, instead of 'executing'? for example, 'connectToExistingProcess' or any name that can explain that it is not spawning new interpreter process but connect to existing process.

Lastly, this branch somehow includes commits not belongs to this contribution, for example afb1214, c4170b5, etc. Could you make sure that this branch does not include commits from other contributions?

SachinJanani · 2016-06-06T16:17:23Z

@Leemoonsoo Even I think adding a checkbox will make more sense.As TravisCI was failing I have merged master in my fork which caused those commits to appear as a part of this contribution I will remove those commits from this contribution

jongyoul · 2016-06-07T06:09:13Z

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

+ * 
+ *
+ */
+public class Constants {


How about finding a way to moving these settings into ZeppelinConfiguration?

@jongyoul Please correct me if I am wrong but I think moving it to ZeppelinConfiguration will make these settings global to all the interpreters.

@SachinJanani You are not wrong, definitely. I think it's better to place all of constants' variables in a same class if we can. How about you?

@jongyoul Ok so you mean just the constants should be moved to ZeppelinConfiguration.But the issue is Zeppelinconfiguration is a part of zeppelin-zengine and adding it as a dependency in zeppelin-interpreter for accessing these constants will cause cyclic dependency.So I think Constants.java should be used to maintain the constants related to interpreter.

@SachinJanani Yes, you are correct, and that's the reason why I ask you. I also think what you implement is utmost solution.

jongyoul · 2016-06-07T06:14:17Z

@SachinJanani Thanks for the contribution. BTW, could you please rebase your change from master? If you merge something from master, it makes review on your PR hard.

…iables

SachinJanani · 2016-06-07T13:15:58Z

@Leemoonsoo @jongyoul This is what will look now:

Please let me know if there are any further changes need to be done

SachinJanani · 2016-06-09T01:43:38Z

@Leemoonsoo @jongyoul One of the TravisCI task is failing but I think its not related to my changes, is there any way I can rerun the travisCI without pushing changes?

jongyoul · 2016-06-09T06:03:32Z

That failure is related to https://issues.apache.org/jira/browse/ZEPPELIN-878, but I have a question. What kind of procedure do you do while you run Zeppelin? Generally, when you stop Zeppelin, all interpreters also stop. Could you please share your cases? This PR will work, but because Zeppelin's interpreter will not be started by itself yet.

SachinJanani · 2016-06-09T06:16:45Z

@jongyoul There can be scenarios where user is starting RemoteInterpreter embedded in his service by calling
RemoteInterpreterServer server = new RemoteInterpreterServer(port) server.start()
So this changes will help user to specify the host and port on which interpreter is executing and zeppelin will connect to that interpreter.This changes will also be helpful when ClusterManager(https://cwiki.apache.org/confluence/display/ZEPPELIN/Cluster+Manager+Proposal) will be implemented.We are starting ZeppelinInterpreter on remote host along with our service.
You are right that when we stop Zeppelin all interpreter will be stop but it can be handled by overriding the RemoteInterpreterServer

jongyoul · 2016-06-09T06:33:22Z

@SachinJanani Thanks for sharing. I also think we should consider running Zeppelin's interpreters in a different machine. This would be a starting point to do this.

SachinJanani · 2016-06-09T06:45:03Z

@jongyoul Yes you are right.But this changes will allow user to connect to existing remote process instead of zeppelin running the interpreter.I think we should create a separate jira for starting zeppelin interpreter on different machine.

jongyoul · 2016-06-09T08:10:32Z

@Leemoonsoo Who's proper reviewer for UI part?

Leemoonsoo · 2016-06-10T02:50:32Z

@jongyoul We have some front-end expert in the community like @corneadoug. But basically anyone in the community can participate review.

Thanks @SachinJanani for the contribution. It looks good to me.

jongyoul · 2016-06-10T03:28:09Z

@Leemoonsoo Yes, I just think there're too many requests for him, so I'm asking you to have another candidate.

corneadoug · 2016-06-10T03:49:38Z

@jongyoul Tested, LGTM.
What about the doc? Should it be part of this PR or a separate one?

jongyoul · 2016-06-10T04:15:57Z

@corneadoug Thanks, and about review, I have been worried that you have been pressed with all of review requests of front-side issues.

SachinJanani · 2016-06-10T05:10:58Z

@jongyoul Should I add section in "docs/manual/interpreters.md" something like "Connecting to the Existing Remote Interpreter"?

jongyoul · 2016-06-10T05:13:16Z

@SachinJanani Yes, it's one of the best place.

SachinJanani · 2016-06-10T05:21:51Z

@jongyoul @corneadoug Done with the docs can you please have a look.

jongyoul · 2016-06-10T05:27:31Z

docs/manual/interpreters.md

+
+## Connecting to the Existing Remote Interpreter
+
+Zeppelin users can start interpreter thread embedded in their service.This will provide flexibility to user to start interpreter on remote host. To start interpreter along with your service you have to create an instance of ``RemoteInterpreterServer`` and start it as follows:


Please add whitespace after "."

jongyoul · 2016-06-10T05:28:35Z

LGTM except minor comments.

AhyoungRyu · 2016-06-10T06:35:30Z

docs/manual/interpreters.md

+
+Zeppelin users can start interpreter thread embedded in their service. This will provide flexibility to user to start interpreter on remote host. To start interpreter along with your service you have to create an instance of ``RemoteInterpreterServer`` and start it as follows:
+
+``RemoteInterpreterServer interpreter=new RemoteInterpreterServer(3789); ``  


@SachinJanani
Currently this part is shown like below

How about changing these 3 code block to like this ?

Then it will be like this

@AhyoungRyu Thanks for review,Yes I think that will look better.Uploaded the changes please have a look

@SachinJanani Yeah thanks for your quick response :) Looks good.

SachinJanani · 2016-06-10T17:39:54Z

Can we merge this if there are no more comments?

Leemoonsoo · 2016-06-11T01:50:25Z

Thanks @SachinJanani for the contribution.
Merge if there're no more discussions.

SachinJanani · 2016-06-12T06:46:16Z

Thanks @Leemoonsoo @jongyoul @AhyoungRyu @corneadoug For reviewing.Can someone please merge this pull request?

SachinJanani added 3 commits June 3, 2016 18:22

Merge pull request #1 from apache/master

53c1eea

Latest update

[ZEPPELIN-940] Allow zeppelin server to connect to already executing …

fbe2346

…Remote Interpreter

Merge branch 'master' of https://github.com/SachinJanani/incubator-ze…

7af8112

…ppelin

Add UI component for the accepting Host and Port when executing optio…

355c1f2

…n is selected

SachinJanani added 3 commits June 6, 2016 18:00

[ZEPPELIN-940] Allow zeppelin server to connect to already executing …

2e30e3d

…Remote Interpreter

Add UI component for the accepting Host and Port when executing optio…

4d51cd9

…n is selected

Merge branch 'master' of https://github.com/SachinJanani/incubator-ze…

9762134

…ppelin

jongyoul reviewed Jun 7, 2016
View reviewed changes

SachinJanani added 2 commits June 7, 2016 17:06

Merge remote-tracking branch 'upstream/master'

c7fdc66

Added checkbox for Connecting to existing process and renamed the var…

84d2347

…iables

Add documentation for connecting to existing remote interpreter

067a06e

jongyoul reviewed Jun 10, 2016
View reviewed changes

Incorporated review comments related to documentation

f57eb78

AhyoungRyu reviewed Jun 10, 2016
View reviewed changes

Changed the Markdown style for code block in document

f279767

asfgit closed this in c9e3a6a Jun 13, 2016


		## Connecting to the Existing Remote Interpreter

		Zeppelin users can start interpreter thread embedded in their service.This will provide flexibility to user to start interpreter on remote host. To start interpreter along with your service you have to create an instance of ``RemoteInterpreterServer`` and start it as follows:


		Zeppelin users can start interpreter thread embedded in their service. This will provide flexibility to user to start interpreter on remote host. To start interpreter along with your service you have to create an instance of ``RemoteInterpreterServer`` and start it as follows:

		``RemoteInterpreterServer interpreter=new RemoteInterpreterServer(3789); ``

Conversation

SachinJanani commented Jun 3, 2016

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:

Uh oh!

Leemoonsoo commented Jun 4, 2016

Uh oh!

SachinJanani commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leemoonsoo commented Jun 5, 2016

Uh oh!

SachinJanani commented Jun 6, 2016

Uh oh!

Leemoonsoo commented Jun 6, 2016

Uh oh!

SachinJanani commented Jun 6, 2016

Uh oh!

jongyoul Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Jun 7, 2016

Uh oh!

SachinJanani commented Jun 7, 2016

Uh oh!

SachinJanani commented Jun 9, 2016

Uh oh!

jongyoul commented Jun 9, 2016

Uh oh!

SachinJanani commented Jun 9, 2016

Uh oh!

jongyoul commented Jun 9, 2016

Uh oh!

SachinJanani commented Jun 9, 2016

Uh oh!

jongyoul commented Jun 9, 2016

Uh oh!

Leemoonsoo commented Jun 10, 2016

Uh oh!

jongyoul commented Jun 10, 2016

Uh oh!

corneadoug commented Jun 10, 2016

Uh oh!

jongyoul commented Jun 10, 2016

Uh oh!

SachinJanani commented Jun 10, 2016

Uh oh!

jongyoul commented Jun 10, 2016

Uh oh!

SachinJanani commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongyoul commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SachinJanani commented Jun 10, 2016

Uh oh!

Leemoonsoo commented Jun 11, 2016

Uh oh!

SachinJanani commented Jun 12, 2016

SachinJanani commented Jun 5, 2016 •

edited

Loading

jongyoul Jun 7, 2016 •

edited

Loading