-
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
[WIP] [ZEPPELIN-1339] [ZEPPELIN-1000] [ZEPPELIN-1338] Note and services per user #1390
Conversation
AFAIK, this issue is already assigned @khalidhuseynov, and he is in progress. @khalidhuseynov Can you share your status? |
this approach is a bit more breaking in terms of changes as well as compatibility with older versions (at least on notebook and storage level). i was working more on a filtering approach on a notes based on permissions which is backward compatible and currently WIP under #1392 . possibly we could discuss each approach's pros and cons and come up with something better. |
@echarles also would you mind rebasing from master? |
@khalidhuseynov Good to discuss with you about what a multiuser note management would be. For the file system repo, this PR will give you:
The note folders are moved one level-down, each user having its workspace. Each user also has its own Interpreter settings and bindings assignment is made possible by keeping separate interpreterfactory, searchservice... per web user session. To ensure backwards compatibility, I propose (not yet implemented) a The other repository implements (azure, github...) need more love with the credential service (each user can have their own credentials). PS: I had a quick look at #1392 - Let's continue this discussion - Depending on the outcomes, we can decide |
@echarles Did you also implement interpreter setting per user in this PR as described in ZEPPELIN-1338 ? |
@@ -22,6 +22,9 @@ | |||
* | |||
*/ | |||
public class AuthenticationInfo { | |||
public static final String ANONYMOUS = "anonynmous"; | |||
public static final AuthenticationInfo ANONYMOUS_AUTHENTICATION_INFO = new AuthenticationInfo(ANONYMOUS); |
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.
checkstyle violation here
so build failed at least because of checkstyle violations. here are some to fix:
Another question is related to what @Leemoonsoo mentioned regarding multiple owners. Actually that's broader question, say user1 added user2 as the reader, or writer for his noteA. then we still need to control each user's view based on note permissions. |
since this also related to #1265 as well as ZEPPELIN-1338 for interpreter multi user environment, maybe @jongyoul could take more detailed look here as well to see possible design, performance, resource utilisation issues |
@zjffdu Correct, this PR also implements ZEPPELIN-1338 User level interpreter setting (I have updated the title and description on this PR). @Leemoonsoo Will check and adapt if needed the behavior for multiple owners and will further digg into #1265 to assess any conflit/divergence. I'd like to second the comment made by @zjffdu in #1265 on the interpreter options complexity. Btw, at the time of the introduction of the current interpreter modes, I already raised my worries on the complexity of those and honestly I am not convinced that I understand them fully (at least at first sight without digging into the code) and I fear that #1265 will make the situation still more complex. Taking back @zjffdu statement: Besides, I think we should think more deeply on the relationship between note, user and interpreter., and I would add also the relation with the SearchService, Helium... and any other services that Zeppelin uses (cc/ @jongyoul) - All these questions also binds to ZEPPELIN-1236 "Multi-user notebook with user controls support" created by @frosiere talking about shared dashboard... Having this discussion on the mailing list independently of any PR would better define the efforst to be taken in each PR. @khalidhuseynov I have the bad habit to build with |
Conflicts: zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinSparkClusterTest.java
Conflicts: notebook/2BWJFTXKJ/note.json
I have introduced a parameter in the URL An alternative would be to intercalate the user in the URL: http://localhost/#/userName/noteId Regarding multiple owners, I would simply drop this feature (just like a file, a note belongs to a unique user). Apart minor navigation glitches, it works pretty well here. I continue to follow #1265 and try to keep this PR orthogonal to what is discussed/implemented in there. I will add a note regarding ZEPPELIN-1144 Zeppelin home page should only list notebooks with read or write permission #1330 - If listing all notes with read access is mandatory, I would have to fallback to the single |
@echarles can you explain more about the |
} | ||
|
||
public NotebookAuthorization getNotebookAuthorization() { | ||
return notebookAuthorization; | ||
} | ||
|
||
public NotebookRepo getNotebookRepo() { |
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.
although might be good idea to have access to it, didn't see where is it used
@echarles I also believe having global/single NotebookAuthorization module would be more relevant |
@corneadoug I will write asap a small document explaining the logic behing this PR (interpreter factory by user, runAs...). |
@echarles I've looked around your PR. I think your PR makes Zeppelin support separate users within a same instance. I have two questions regarding current behaviour. The first is about collaborative mode. If some users want to share their notebook to others which are able to login in same Zeppelin instance, and enable them to run it, how can user do that? The second one is about running interpreter instance. For now, Zeppelin supports three mode for different notes to run an interpreter, 'shared', 'scoped', 'isolated'. But in your PR, because you make interpreterFactory divided by users, all interpreters run a 'isolated' mode for all users. This means that if hundred users run simple markdown interpreter, Zeppelin launches same number of JVM on a same host. My PR considers that situation but your PR divide interpreterFactory by user, then will break my consideration. What do you think of it? And complexity of configuration, @corneadoug will help to make intuitive menu. |
…eppelin.interpreter.peruser.factories configuration
Conflicts: zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/InterpreterFactoryTest.java
I didn't look through the PR, but according @jongyoul 's last comments. If only |
I have a prototype that supports the 3 modes. Give me a few days to polish and commit. |
@echarles you can also push gradually to facilitate review process |
close #83 close #86 close #125 close #133 close #139 close #146 close #193 close #203 close #246 close #262 close #264 close #273 close #291 close #299 close #320 close #347 close #389 close #413 close #423 close #543 close #560 close #658 close #670 close #728 close #765 close #777 close #782 close #783 close #812 close #822 close #841 close #843 close #878 close #884 close #918 close #989 close #1076 close #1135 close #1187 close #1231 close #1304 close #1316 close #1361 close #1385 close #1390 close #1414 close #1422 close #1425 close #1447 close #1458 close #1466 close #1485 close #1492 close #1495 close #1497 close #1536 close #1545 close #1561 close #1577 close #1600 close #1603 close #1678 close #1695 close #1739 close #1748 close #1765 close #1767 close #1776 close #1783 close #1799
What is this PR for?
This implements "notes per user" and "Multiple-users on a single WEB server" - For implementation reasons (the static fields present on the zeppelin server, and the need for each users to bind/configure their own note to the interpereters managed by the InterpreterFactory), this PR implements both - In other words, it would have been too difficult to decouple both).
I also implements "User level interpreter setting".
What type of PR is it?
[Feature]
Todos
What is the Jira issue?
How should this be tested?
Logon / Logout with different users and check that the notes are different.
Screenshots (if appropriate)
Questions: