-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[ZEPPELIN-1690] - ZeppelinHubNotebookRepo multy user handling #1635
[ZEPPELIN-1690] - ZeppelinHubNotebookRepo multy user handling #1635
Conversation
@anthonycorbacho thanks for enhancements! i started looking into this one and first of all CI looks like failing with checkstyle issues. Also some methods seem to be missing, and proper jira ticket would be appreciated :) let me know if i can help somehow |
@khalidhuseynov as you can see its a wip, i will let you know when its ready for review |
- Now takes a token on every http request
@@ -157,6 +161,9 @@ protected User authenticateUser(String requestBody) { | |||
ZeppelinServer.notebookWsServer.broadcastReloadedNoteList( | |||
new org.apache.zeppelin.user.AuthenticationInfo(account.login), userAndRoles); | |||
|
|||
// Add ZeppelinHub user_session token this singleton map, this will help ZeppelinHubRepo | |||
// to get specific information about the current user. | |||
UserSessionContainer.instance.setSession(account.login, userSession); |
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 this setSession
should go few lines above, right after catch
statement, since there's broadcastReloadedNoteList
called afterwards that will require
CI is green, please review otherwise merging it :) |
thanks for the fix, LGTM! |
What is this PR for?
This PR bring multi user handling to ZeppelinHubNotebookRepo.
What type of PR is it?
[Improvement ]
What is the Jira issue?
Questions: