Skip to content

User in interpreter context - #705

Closed
prabhjyotsingh wants to merge 14 commits into
apache:masterfrom
prabhjyotsingh:UserInInterpreterContext
Closed

User in interpreter context#705
prabhjyotsingh wants to merge 14 commits into
apache:masterfrom
prabhjyotsingh:UserInInterpreterContext

Conversation

@prabhjyotsingh

Copy link
Copy Markdown
Contributor

What is this PR for?

The goal of the PR is to pass userName/userInfo from front-end to interpreters.
The PR uses Shiro authentication(#586).

What type of PR is it?

Improvement

Todos

  • - Change RemoteInterpreterService.thrift signature
  • - Change InterpreterContext/RemoteInterpreterContext signature
  • - Modify all existing interpreters to above signature

Is there a relevant Jira issue?

N/A

@prabhjyotsingh
prabhjyotsinghforce-pushed the UserInInterpreterContext branch from f228b6d to 60e0a6eCompareFebruary 9, 2016 10:20
@prabhjyotsingh
prabhjyotsinghforce-pushed the UserInInterpreterContext branch from 60e0a6e to 320790cCompareFebruary 9, 2016 10:56
@rconline

Copy link
Copy Markdown
Contributor

This is a requisite first step for getting to multi-user scenarios. I believe this should get discussed a little bit in terms of approach.

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

This is ready for review.

@felixcheung

Copy link
Copy Markdown
Member

@rconline how would you want to discuss? Should we open a JIRA - or a JIRA parent with subtests for this multi-user auth use cases?

@felixcheung

Copy link
Copy Markdown
Member

@prabhjyotsingh could you explain more on the reason for this change?
Also this seems like a somewhat big change - could you open a JIRA on this so it could be tracked?

@rconline

Copy link
Copy Markdown
Contributor

@felixcheung@prabhjyotsingh The purpose of this discussion should be to get to an approach that allows us zeppelin to do a couple of things - pass user identity to the underlying spark cluster, and thereby support multi-tenancy. There is JIRA open - https://issues.apache.org/jira/browse/ZEPPELIN-645 which we could use to discuss this further. Does that make sense?

@Leemoonsoo

Copy link
Copy Markdown
Member

I think it make sense to let access authentication info through InterpreterContext.
But i think authentication info will be more than a user name in the future. It can have some credentials/token stuff from Authentication system (shiro), so interpreter process can reuse it without asking authentication again, when they're required.

So my suggestion is, create such class AuthenticationInfo, and pass it into InterpreterContext instead of String userName, for further extension in the future.

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

Thank you for the suggestion @Leemoonsoo. Have added AuthenticationInfo class, and made relevant changes.

@Leemoonsoo

Copy link
Copy Markdown
Member

I have tried and it works well.
One thing is, org.apache.zeppelin.display package supposed to have Display System related codes.
I think AuthenticationInfo is less related to Display System. Could you suggest other package name?

@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

Thanks you @Leemoonsoo, that does make a lot of sense.

@Leemoonsoo

Copy link
Copy Markdown
Member

Thanks @prabhjyotsingh.
It looks good to me.

@jongyoul

Copy link
Copy Markdown
Member

LGTM

@felixcheung

Copy link
Copy Markdown
Member

Let's rebase and merge this?

…ntext
# Conflicts:
#	zeppelin-server/src/test/java/org/apache/zeppelin/integration/ZeppelinIT.java
@prabhjyotsingh

Copy link
Copy Markdown
ContributorAuthor

Thank you for reviewing this. Have merged this with master.

@prabhjyotsingh
prabhjyotsingh deleted the UserInInterpreterContext branch June 1, 2016 15:46
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@rconline@felixcheung@Leemoonsoo@jongyoul