ZEPPELIN-1293. Livy Interpreter: Automatically attach or create to a new session by zjffdu · Pull Request #1861 · apache/zeppelin

zjffdu · 2017-01-06T11:09:26Z

What is this PR for?

By default, livy session will expire after one hour. This PR would create session automatically for user if session is expired, and would also display the session expire information in frontend. The expire message would only display at the first time of session recreation, after that the message won't be displayed.

What type of PR is it?

[Improvement ]

Todos

- Task

What is the Jira issue?

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

How should this be tested?

Tested manually.

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-01-06T12:36:16Z

@prabhjyotsingh @felixcheung Please help review.

…new session

felixcheung · 2017-01-06T23:17:22Z

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


what's our coding style guide on multiple imports? is it clearer to list them out

This is done by intellij which would use * when imports more than 5 classes in one package. I think it should be fine to do that.

felixcheung · 2017-01-06T23:22:07Z

+ stmtInfo = executeStatement(new ExecuteRequest(code));
+ } catch (SessionNotFoundException e) {
+ LOGGER.warn("Livy session {} is expired, new session will be created.", sessionInfo.id);
+ this.sessionExpired = true;


instead of saving this as a state, would be more self-contain to pass this as a flag to appendSessionExpire below?

Good idea !

felixcheung · 2017-01-06T23:23:20Z

+ result2.add(message.getType(), message.getData());
+ }
+ result2.add(InterpreterResult.Type.HTML,
+ "<hr/><font color=\"red\">Previous session is expired, new session is created.</font>");


perhaps it is better for this message to be at the top of the result output, rather than at the bottom of it? doesn't it require the user to scroll down if he has a long output?

hmm, make sense to put it at the top

zjffdu · 2017-01-09T09:00:38Z

@felixcheung I have addressed the comments and also fix one bug in LivySparkSQLInterpreter, and update the screenshot of testing in PR description.

prabhjyotsingh · 2017-01-09T09:44:10Z

Tested locally, looks good.

felixcheung · 2017-01-10T07:46:18Z

 resMsg.append("\n");
+ if (rows[3].indexOf("+") == 0) {
+
+ } else {


this exists before, but this construct is a bit strange?
if (rows[3].indexOf("+") == 0) { } else {

I don't know about this logic, @prabhjyotsingh is the original author who might know this.

felixcheung · 2017-01-10T07:48:55Z

LGTM.
what is the bug in LivySparkSQLInterpreter - should we have a JIRA for it?

zjffdu · 2017-01-10T08:00:21Z

The bug in LivySparkSQLInterpreter is caused by this PR and fixed in this PR. LivySparkSQLInterpreter delegate LivySparkInterpreter to generate InterpreterResult and then convert it to table type. So here since we make it as multiple InterpreterResult, we should only convert the InterpreterMessage with text type that is for sql output.

zjffdu · 2017-01-12T03:53:22Z

ping @felixcheung any more comments ?

felixcheung · 2017-01-12T06:47:12Z

nope,
merging if no more comment

felixcheung · 2017-01-13T04:16:47Z

 result2.add(InterpreterResult.Type.HTML,
- "<font color=\"red\">Previous session is expired, new session is created.</font>");
+ "<font color=\"red\">Previous session is expired, new session is created. " +
+ "Paragraphes that depend on this paragraph need to be re-executed !" + "</font>");


I think it should be Paragraphs. Also, let remove the space before !

Thanks, addressed.

felixcheung · 2017-01-13T07:33:24Z

merging if no more comment

…new session ### What is this PR for? By default, livy session will expire after one hour. This PR would create session automatically for user if session is expired, and would also display the session expire information in frontend. The expire message would only display at the first time of session recreation, after that the message won't be displayed. ### What type of PR is it? [Improvement ] ### Todos * [ ] - Task ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-1293 ### How should this be tested? Tested manually. ![image](https://cloud.githubusercontent.com/assets/164491/21761175/2473c0c0-d68c-11e6-8f39-9e87333c6168.png) ### 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#1861 from zjffdu/ZEPPELIN-1293 and squashes the following commits: e174593 [Jeff Zhang] minor update on warning message 30c3569 [Jeff Zhang] address comments 88f0d9a [Jeff Zhang] ZEPPELIN-1293. Livy Interpreter: Automatically attach or create to a new session

zjffdu · 2017-01-17T09:43:32Z

@felixcheung @minahlee Could any of you help to merge it into branch-0.7 as well ? Since this is only livy interpreter specific, so I don't think it would affect the 0.7 release.

zjffdu · 2017-01-17T09:47:08Z

@felixcheung @minahlee , Sorry my mistake, It has been merged into branch-0.7. @minahlee I notice you make the jira fixversion as 0.8, I add 0.7 as well.

zjffdu mentioned this pull request Jan 6, 2017
[ZEPPELIN-1293] Re-create Livy session if it's lost #1447
Closed

ZEPPELIN-1293. Livy Interpreter: Automatically attach or create to a …
88f0d9a
…new session

zjffdu force-pushed the ZEPPELIN-1293 branch from 852f982 to 88f0d9aCompare January 6, 2017 12:37

felixcheung reviewed Jan 6, 2017
View reviewed changes

address comments
30c3569

felixcheung reviewed Jan 10, 2017
View reviewed changes

felixcheung reviewed Jan 13, 2017
View reviewed changes

zjffdu force-pushed the ZEPPELIN-1293 branch from 993a039 to 566e942Compare January 13, 2017 04:59

minor update on warning message
e174593

zjffdu force-pushed the ZEPPELIN-1293 branch from 566e942 to e174593Compare January 13, 2017 10:58

asfgit closed this in 00742ffJan 14, 2017

Uh oh!

Conversation

zjffdu commented Jan 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

zjffdu commented Jan 6, 2017

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!

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Jan 9, 2017

Uh oh!

prabhjyotsingh commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Jan 10, 2017

Uh oh!

zjffdu commented Jan 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffdu commented Jan 12, 2017

Uh oh!

felixcheung commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Jan 13, 2017

Uh oh!

zjffdu commented Jan 17, 2017

Uh oh!

zjffdu commented Jan 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zjffdu commented Jan 6, 2017•
edited
Loading

zjffdu commented Jan 10, 2017•
edited
Loading

zjffdu commented Jan 17, 2017•
edited
Loading