Skip to content

[Zeppelin-1022] Apply new mechanism to LivyInterpreter - #1028

Closed
prabhjyotsingh wants to merge 5 commits into
apache:masterfrom
prabhjyotsingh:ZEPPELIN-1022
Closed

[Zeppelin-1022] Apply new mechanism to LivyInterpreter#1028
prabhjyotsingh wants to merge 5 commits into
apache:masterfrom
prabhjyotsingh:ZEPPELIN-1022

Conversation

@prabhjyotsingh

Copy link
Copy Markdown
Contributor

What is this PR for?

Apply new mechanism to LivyInterpreter

What type of PR is it?

[Improvement]

Todos

  • - Apply new mechanism to LivyInterpreter
  • - rename zeppelin.livy.url to livy.host.url to make all params look livy.*
  • - surround interpreterContext.getAuthenticationInfo().getUser() with ""

What is the Jira issue?

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

CI fails for unrelated changes

@bzz

bzz commented Jun 17, 2016

Copy link
Copy Markdown
Member

Looks good to me

@prabhjyotsingh if we could point what is the reason of CI failure and link to either PR or existing JIRA issue that handles it - that could help improving our infrastructure a lot

\cc @jongyoul as an author of new interpreter registration system for a reivew

String confData = gson.toJson(conf);

String json = executeHTTP(property.getProperty("zeppelin.livy.url") + "/sessions",
String json = executeHTTP(property.getProperty("livy.host.url") + "/sessions",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

my thought would be this:

  • any property needed by Zeppelin (such as to communicate with another server), it should start with "zeppelin.*"
  • any property not used by Zeppelin but by a separate server or framework (eg. spark), it should start with "spark.*"
  • the reason is the interpreter can then pass-through properties as-is, for example, spark interpreter can pass all "spark." property to SparkConf (and skip all the zeppelin. ones!)

I think we try to roughly follow that pattern (at least as much as I have seen)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fair point, I'll revert this change.

@jongyoul

Copy link
Copy Markdown
Member

I've tested it. LGTM

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

@bzz I was referring to #1034, ZEPPELIN-1009.

Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 104.524 sec - in org.apache.zeppelin.integration.ZeppelinIT
Results :
Tests in error: ParagraphActionsIT.testRemoveButton:153 » NoSuchElement Unable to locate eleme...
ParagraphActionsIT.testCreateNewButton:86 » NoSuchElement Unable to locate ele...

@jongyoul Thank you for testing it out.

@r-kamath

Copy link
Copy Markdown
Member

@prabhjyotsingh LGTM

surround interpreterContext.getAuthenticationInfo().getUser() with ""

Thanks

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

Will merge this if no more discussion.

@prabhjyotsingh
prabhjyotsingh deleted the ZEPPELIN-1022 branch August 15, 2017 19:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@prabhjyotsingh@bzz@jongyoul@r-kamath@felixcheung