[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast #1392

khalidhuseynov · 2016-09-01T07:52:45Z

What is this PR for?

This PR addresses part of multi-user note management in Zeppelin. One of the tasks namely listing notes per user on Zeppelin start was addressed in #1330. However that PR didn't solve all problems, and reloading notes was incomplete as well as socket broadcast was not user aware ZEPPELIN-1437, ZEPPELIN-1438. This PR addresses those issue.

What type of PR is it?

Improvement

Todos

- list notes per user on reload
- broadcast per user (multicast)
- tests
- use authorization module to filter notes on sync
- broadcast on permissions change
- discussion and review

What is the Jira issue?

Zeppelin-1437, ZEPPELIN-1438

How should this be tested?

Start Zeppelin
Login as user1, and user2 on different windows
Each user should be able to see their own note workbench
If note changed to private (readers, writers not empty), that note should disappear from others note workbench.

Screenshots (if appropriate)

Questions:

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

khalidhuseynov · 2016-09-20T17:01:53Z

this is ready for review

that's to sync only user workbench notes

khalidhuseynov · 2016-09-21T05:20:23Z

rebased and CI is green

echarles · 2016-09-21T13:51:52Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NotebookAuthorization.java


-  public NotebookAuthorization(ZeppelinConfiguration conf) {
-    this.conf = conf;
+  public static NotebookAuthorization init(ZeppelinConfiguration config) {


Why moving from constructor to init method (factory method pattern)?
Especially, the conf object is static, so calling multiple time init will override the conf field. The state of the NotebookAuthorization will not be consistent.

@echarles good point. actually that's more of a singleton pattern that gets instantiated once on init and can be retrieved by getInstance further on. the purpose of it was to enable other modules (e.g. storage in zengine) not having direct reference to notebookAuthorization, to be able to use its services without changing api. I believe for longer term authorization and storage modules can be refactored in a way that we don't need that pattern.

regarding overriding the conf field, you're right. I addressed it in 9cf1d88 so that it gets instantiated only once.

khalidhuseynov · 2016-09-27T05:49:39Z

i've addressed zeppelin-1438 here as well in last commit a2ce268, so user workbench should be updating user lists without manual reload, ready for review

Leemoonsoo · 2016-10-17T08:21:05Z

Tested and LGTM.

Merge if there're no more discussions

anthonycorbacho · 2016-10-17T08:53:06Z

@Leemoonsoo is it possible to merge it in couple of hour? some of my work will depends on this PR :)

…broadcast ### What is this PR for? This PR addresses part of multi-user note management in Zeppelin. One of the tasks namely listing notes per user on Zeppelin start was addressed in apache#1330. However that PR didn't solve all problems, and reloading notes was incomplete as well as socket broadcast was not user aware [ZEPPELIN-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438). This PR addresses those issue. ### What type of PR is it? Improvement ### Todos * [x] - list notes per user on reload * [x] - broadcast per user (multicast) * [x] - tests * [x] - use authorization module to filter notes on sync * [x] - broadcast on permissions change * [ ] - discussion and review ### What is the Jira issue? [Zeppelin-1437](https://issues.apache.org/jira/browse/ZEPPELIN-1437), [ZEPPELIN-1438](https://issues.apache.org/jira/browse/ZEPPELIN-1438) ### How should this be tested? 1. Start Zeppelin 2. Login as user1, and user2 on different windows 3. Each user should be able to see their own note workbench 4. If note changed to private (readers, writers not empty), that note should disappear from others note workbench. ### Screenshots (if appropriate) ![reload_broadcast](https://cloud.githubusercontent.com/assets/1642088/18679507/e4a0161c-7f9a-11e6-9d57-0930abf4b780.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? yes Author: Khalid Huseynov <[email protected]> Closes apache#1392 from khalidhuseynov/feat/multi-user-notes and squashes the following commits: a2ce268 [Khalid Huseynov] broadcast note list on perm update - zeppelin-1438 9cf1d88 [Khalid Huseynov] fix init not to initialize every time 17eae84 [Khalid Huseynov] bugfix: add precondition for NP 781207e [Khalid Huseynov] bugfix: reload only once 537cc0e [Khalid Huseynov] apply filter from authorization in sync 09e6723 [Khalid Huseynov] notebookAuthorization as singleton 9427e62 [Khalid Huseynov] multicast fine grained note lists to users instead of broadcast 6614e2b [Khalid Huseynov] improve tests 1399407 [Khalid Huseynov] remove unused imports d9c3bc9 [Khalid Huseynov] filter reload using predicates 92f37f5 [Khalid Huseynov] substitute old getAllNotes(subject) with new implementation b7f19c9 [Khalid Huseynov] separate getAllNotes() and getAllNotes(subject) 17e2d4c [Khalid Huseynov] first draft

This was referenced Sep 1, 2016

[WIP] [ZEPPELIN-1339] [ZEPPELIN-1000] [ZEPPELIN-1338] Note and services per user #1390

Closed

[ZEPPELIN-1144]Zeppelin home page should only list notebooks with read or write permission #1330

Closed

khalidhuseynov force-pushed the feat/multi-user-notes branch 8 times, most recently from 1f25a5a to 469360b Compare September 20, 2016 13:42

khalidhuseynov changed the title ~~WIP [ZEPPELIN-1339] Multi-user note management~~ [ZEPPELIN-1437] Multi-user note management Sep 20, 2016

khalidhuseynov changed the title ~~[ZEPPELIN-1437] Multi-user note management~~ [ZEPPELIN-1437] Multi-user note management - user aware reload broadcast Sep 20, 2016

khalidhuseynov closed this Sep 20, 2016

khalidhuseynov reopened this Sep 20, 2016

khalidhuseynov added 10 commits September 21, 2016 11:44

first draft

17e2d4c

separate getAllNotes() and getAllNotes(subject)

b7f19c9

substitute old getAllNotes(subject) with new implementation

92f37f5

filter reload using predicates

d9c3bc9

remove unused imports

1399407

improve tests

6614e2b

multicast fine grained note lists to users instead of broadcast

9427e62

notebookAuthorization as singleton

09e6723

apply filter from authorization in sync

537cc0e

that's to sync only user workbench notes

bugfix: reload only once

781207e

khalidhuseynov force-pushed the feat/multi-user-notes branch from 2eeb873 to 781207e Compare September 21, 2016 02:47

bugfix: add precondition for NP

17eae84

echarles reviewed Sep 21, 2016

View reviewed changes

fix init not to initialize every time

9cf1d88

khalidhuseynov closed this Sep 27, 2016

khalidhuseynov reopened this Sep 27, 2016

broadcast note list on perm update - zeppelin-1438

a2ce268

khalidhuseynov force-pushed the feat/multi-user-notes branch from 094de2e to a2ce268 Compare September 27, 2016 05:04

khalidhuseynov changed the title ~~[ZEPPELIN-1437] Multi-user note management - user aware reload broadcast~~ [ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast Sep 27, 2016

asfgit closed this in b77f9ea Oct 18, 2016

khalidhuseynov deleted the feat/multi-user-notes branch October 18, 2016 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast #1392

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast #1392

Uh oh!

khalidhuseynov commented Sep 1, 2016 •

edited

Loading

Uh oh!

khalidhuseynov commented Sep 20, 2016

Uh oh!

khalidhuseynov commented Sep 21, 2016

Uh oh!

echarles Sep 21, 2016 •

edited

Loading

Uh oh!

khalidhuseynov Sep 24, 2016

Uh oh!

khalidhuseynov commented Sep 27, 2016

Uh oh!

Leemoonsoo commented Oct 17, 2016

Uh oh!

anthonycorbacho commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast #1392

[ZEPPELIN-1437, 1438] Multi-user note management - user aware reload broadcast #1392

Uh oh!

Conversation

khalidhuseynov commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

khalidhuseynov commented Sep 20, 2016

Uh oh!

khalidhuseynov commented Sep 21, 2016

Uh oh!

echarles Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khalidhuseynov Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

khalidhuseynov commented Sep 27, 2016

Uh oh!

Leemoonsoo commented Oct 17, 2016

Uh oh!

anthonycorbacho commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

khalidhuseynov commented Sep 1, 2016 •

edited

Loading

echarles Sep 21, 2016 •

edited

Loading