Skip to content

ZEPPELIN-2224. Throw more meaningful exception when kerberos is enabled in livy interpreter #2104

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.

Sign up for GitHub

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

Closed
zjffdu wants to merge 2 commits into from
Closed

ZEPPELIN-2224. Throw more meaningful exception when kerberos is enabled in livy interpreter #2104

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0221193
ZEPPELIN-2224. Throw more meaningful exception when kerberos is enabl…
zjffdu Mar 7, 2017
67feb4c
code cleanup
zjffdu Mar 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions livy/src/main/java/org/apache/zeppelin/livy/BaseLivyInterprereter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.conn.ssl.SSLContexts;
import org.apache.http.impl.client.HttpClients;
import org.apache.commons.lang.exception.ExceptionUtils;
import org.apache.zeppelin.interpreter.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -367,13 +368,11 @@ private InterpreterResult getResultFromStatementInfo(StatementInfo stmtInfo,
}

if (displayAppInfo) {
//TODO(zjffdu), use multiple InterpreterResult to display appInfo
InterpreterResult interpreterResult = new InterpreterResult(InterpreterResult.Code.SUCCESS);
interpreterResult.add(InterpreterResult.Type.TEXT, result);
String appInfoHtml = "<hr/>Spark Application Id: " + sessionInfo.appId + "<br/>"
+ "Spark WebUI: <a href=\"" + sessionInfo.webUIAddress + "\">"
+ sessionInfo.webUIAddress + "</a>";
LOGGER.info("appInfoHtml:" + appInfoHtml);
interpreterResult.add(InterpreterResult.Type.HTML, appInfoHtml);
return interpreterResult;
} else {
Expand Down Expand Up @@ -484,6 +483,8 @@ private String callRestAPI(String targetURL, String method, String jsonData)
if (cause.getResponseBodyAsString().matches(SESSION_NOT_FOUND_PATTERN)) {
throw new SessionNotFoundException(cause.getResponseBodyAsString());
}
throw new LivyException(cause.getResponseBodyAsString() + "\n"
+ ExceptionUtils.getFullStackTrace(ExceptionUtils.getRootCause(e)));
Copy link
Member

@felixcheung felixcheung Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to bubble up to the face of the user with the whole stack?
perhaps logger.error it instead?

Copy link
Contributor Author

@zjffdu zjffdu Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the default behavior of zeppelin interpreter. See
https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java#L191

Copy link
Contributor Author

@zjffdu zjffdu Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixcheung Any more comment ?

Copy link
Member

@felixcheung felixcheung Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get it though. Here we are creating a new exception with the full stack trace of the inner/original exception in the exception message itself. In the example you point to setResult(e.getMessage()) would then get the whole message + stack and set that as the result of the paragraph, which would be huge?

Copy link
Contributor Author

@zjffdu zjffdu Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if no error message is speficed explicitly, job stack strace will be used as the error message, see
https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java#L331

Copy link
Member

@felixcheung felixcheung Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, ok, why not also include the original exception?
https://docs.oracle.com/javase/7/docs/api/java/lang/Exception.html#Exception(java.lang.String,%20java.lang.Throwable)

Copy link
Contributor Author

@zjffdu zjffdu Mar 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation use the original exception without the ResponseBody, the purpose of this PR is to add the ResponseBody to exception message, so that in frontend, the error message is more meaningful in kerberos enviroment. See the comparison in PR description.

Copy link
Contributor Author

@zjffdu zjffdu Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @felixcheung

}
throw new LivyException(e);
}
Expand Down