-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-2949] Allow custom Spark UI URL #2596
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
felixcheung
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.
seems like a reasonable approach
there's one test failure, could you check?
|
Thanks @felixcheung. Test looks unrelated but I'll double check. Not sure how much we want to document on how to work with Spark behind reverse proxy but I can add that by setting properties we have a working solution:
https://issues.apache.org/jira/browse/SPARK-20044 is preventing us from using a path prefix and it is assumed we are routing from spark port to reverseProxyUniqueKey |
|
Not sure what we can do to have the tests passing: |
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.
test failures looks to be unrelated (sorry typo), but could you rebase your PR and try tests again?
testGetFactory(org.apache.zeppelin.interpreter.InterpreterFactoryTest) Time elapsed: 0.584 sec <<< FAILURE!
org.junit.ComparisonFailure: expected:<...eppelin.interpreter.[EchoInterpreter]> but was:<...eppelin.interpreter.[mock.MockInterpreter1]>
docs/interpreter/spark.md
Outdated
| <tr> | ||
| <td>zeppelin.spark.uiWebUrl</td> | ||
| <td></td> | ||
| <td>Override Spark UI default URL</td> |
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 you add a bit more on what are the condition to set this, example value etc..?
|
Thanks @felixcheung and apologies for the delay on this. |
felixcheung
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.
great, sorry a few minor comment I missed the last time
docs/interpreter/spark.md
Outdated
| <tr> | ||
| <td>zeppelin.spark.uiWebUrl</td> | ||
| <td></td> | ||
| <td>Overrides Spark UI default URL (ex: http://{hostName}/{uniquePath)</td> |
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.
should this be {uniquePath) -> {uniquePath})
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.
I think we should clarify that the value should be a full URL
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.
Fixed on latest commit
| } | ||
|
|
||
| String sparkUrlProp = property.getProperty("zeppelin.spark.uiWebUrl", ""); | ||
| if (!sparkUrlProp.isEmpty()) { |
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.
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.
I agree. Fixed on latest commit
felixcheung
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.
LGTM
|
merging if no more comment |
### What is this PR for? Fix build interpreters (apark and R) which broken after apache#2592 and apache#2596 ### What type of PR is it? [Hot Fix] ### How should this be tested? build interpreters ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: tinkoff-dwh <tinkoff.dwh@gmail.com> Closes apache#2630 from tinkoff-dwh/fix_r_interpretator and squashes the following commits: 0c9828b [tinkoff-dwh] [HOTFIX] fix build spark and R interpreters
What is this PR for?
Allow an override of default Spark logic in building the URL path and redirect the Zeppelin user directly to a configured reverse proxy path
What type of PR is it?
[Improvement]
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: