-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-1293] Re-create Livy session if it's lost #1447
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
Conversation
|
@spektom Thanks for the contribution, I am working on a PR about integration test of livy interpreter, could you mind to wait for that PR and then add integration test to that ? |
|
Of course, it would be great.
|
|
@spektom and @zjffdu is it possible to add a null check also. I've been doing some debugging over the past few days and certain situations can cause nulls to be returned and in theory if a null is being returned the session is dead. So instead of this: This: |
|
Also I have some concerns on using Exceptions to as goto's as there is definite performance issues that occur by using exceptions... http://javarevisited.blogspot.com/2013/03/0-exception-handling-best-practices-in-Java-Programming.html. Specifically since code inside of the try catch cannot be optimized. |
12ee62d to
5fdb6df
Compare
|
@gss2002 you shouldn't be concerned about performance as this code only runs when command is executed. |
|
@spektom this fix is good. Did some extensive load testing with it this AM and it solves the session expiration issues. Thanks for the contribution |
| throw new LivyException("Error executing command in Livy", e); | ||
| } | ||
| if (json == null || json.matches("^(\")?Session (\'[0-9]\' )?not found(.?\"?)$")) { | ||
| throw new LivyNoSessionException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add some logging when this happens?
purechoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now, "Session not found" pattern matched,
session id between 1 and 10.
| } catch (Exception e) { | ||
| throw new LivyException("Error executing command in Livy", e); | ||
| } | ||
| if (json == null || json.matches("^(\")?Session (\'[0-9]\' )?not found(.?\"?)$")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to be change like this, isn't it?
json.matches("^(")?Session ('[0-9]' )?not found(.?"?)$")
5fdb6df to
d7f1da5
Compare
|
@spektom / @zjffdu @purechoc there is definitely an additional condition. Not sure if it's because the ConcurrentHashMaps are not being used correctly. But the exception doesn't get caught completely or correctly at times with the fix proposed here.. ERROR [2016-10-19 14:19:57,638]({pool-2-thread-11} LivyHelper.java[executeHTTP]:378) - Error with 404 StatusCode: "Session '9' not found." |
|
@gss2002 are you using latest code from the branch? I can't find org.apache.zeppelin.livy.LivyHelper.interpretInput(LivyHelper.java:189) invocation at org.apache.zeppelin.livy.LivySparkInterpreter.interpret(LivySparkInterpreter.java:106) as it's shown in your stack trace. |
|
@spektom I think what happens here is this code fires.. which has nothing to do with the fix here.. in LivySparkInterpreter: That gets called before the NoSessionException occurs.. And then in |
d7f1da5 to
39c5561
Compare
|
@spektom just tested against my build by catching the exception and rethrowing.. It definitely solves the issue. } public InterpreterResult interpret(String stringLines, The code base I'm using is here with you patch and a few of @zjffdu patches and one of mine for NestedRuntimeException for 404's with KerberosTemplate: https://github.com/gss2002/zeppelin/blob/GSS_PROD_BUILD/livy/src/main/java/org/apache/zeppelin/livy/LivyHelper.java |
|
Sorry for late response. @spektom Could you add some additional message of session recreation to be displayed in frontend. Because if session is recreated, we may need to rerun all the paragraphs, without knowing session recreationusers may be confused for errors like some variables are not defined. |
39c5561 to
522e62d
Compare
|
LGTM. it's working nicely in my environment. (zeppelin 0.7 + livy 2.1) |
|
@spektom Do you want to rebase it and continue work on it ? |
|
@spektom Sorry, I didn't get your reply, I create another PR for it #1861 |
close #83 close #86 close #125 close #133 close #139 close #146 close #193 close #203 close #246 close #262 close #264 close #273 close #291 close #299 close #320 close #347 close #389 close #413 close #423 close #543 close #560 close #658 close #670 close #728 close #765 close #777 close #782 close #783 close #812 close #822 close #841 close #843 close #878 close #884 close #918 close #989 close #1076 close #1135 close #1187 close #1231 close #1304 close #1316 close #1361 close #1385 close #1390 close #1414 close #1422 close #1425 close #1447 close #1458 close #1466 close #1485 close #1492 close #1495 close #1497 close #1536 close #1545 close #1561 close #1577 close #1600 close #1603 close #1678 close #1695 close #1739 close #1748 close #1765 close #1767 close #1776 close #1783 close #1799
What is this PR for?
This patch restarts Livy session when it's lost.
What type of PR is it?
Improvement/Bug Fix
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1293
How should this be tested?
livy.server.session.timeoutparameter in LivyQuestions: