Skip to content

[Zeppelin 796] Duplicated notebook names should not be allowed - #1464

Closed
ghost wants to merge 21 commits into
masterfrom
unknown repository
Closed

[Zeppelin 796] Duplicated notebook names should not be allowed#1464
ghost wants to merge 21 commits into
masterfrom
unknown repository

Conversation

@ghost

Copy link
Copy Markdown

What is this PR for?

When a notebook is created/cloned/imported the title/name of the new notebook should not be duplicate of the existing notebook's title/name

What type of PR is it?

Improvement

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-796

How should this be tested?

  1. Create a notebook with name 'Note', Notebook should allow user to create a note
  2. When a user try to create/clone/import a notebook with name 'Note', Notebook should not
    allow user to create note.

Screenshots (if appropriate)

Screenshot of error message displayed during create/clone notebook
create

Screenshot of error message displayed during import notebook
import

Questions:

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

addConnectionToNote(note.getId(), (NotebookSocket) conn);
conn.send(serializeMessage(new Message(OP.NEW_NOTE).put("note", note)));
broadcastNoteList(subject);
} catch (DuplicateNameException dne) {

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.

shouldn't it rethrow the exception? would the caller expect an exception like IOException?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For all websocket requests, we are sending responses from notebookServer class. So, i believe as a standard we should handle the response in the same file. So, rethrowing the exception wouldn't serve that purpose.

raja-imagineaand others added 17 commits September 28, 2016 14:31
### What is this PR for?
This PR is for refactoring code for JDBCInterpreter.
There is no putting 'Connection' to 'propertyKeyUnusedConnectionListMap' anywhere in the original code.
### What type of PR is it?
Improvement
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1405
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
Author: astroshim <hsshim@nflabs.com>
Closes#1396 from astroshim/ZEPPELIN-1405 and squashes the following commits:
b07e162 [astroshim] add checking connection is null
f6998c2 [astroshim] Merge branch 'master' into ZEPPELIN-14051862ae6 [astroshim] Merge branch 'master' into ZEPPELIN-1405efc2bfc [astroshim] rebase
21217a7 [astroshim] fix indentation.
4d4f85c [astroshim] refactoring code of close()
9f1e368 [astroshim] replace ConnectionPool
4dabbcc [astroshim] wip) changing to use dbcp
12dd7cb [astroshim] remove propertyKeyUnusedConnectionListMap map
@khalidhuseynov

Copy link
Copy Markdown
Member

@rajarajan-g why do you think it's a problem having same note title/name with existing one. could you give more context?

@ghost

Copy link
Copy Markdown
Author

@khalidhuseynov : Lets say if i create a note with name 'Note'. If zeppelin allows me to create a new note with name 'Note', then after sometime when i search for one of this note, I am going to get confused here.

@ghost

Copy link
Copy Markdown
Author

Going to reopen the pull request to trigger CI build

@ghostghost closed this Sep 29, 2016
@ghostghost reopened this Sep 29, 2016
@ghost

Copy link
Copy Markdown
Author

CI build is green & success. Please review

@jongyoul

Copy link
Copy Markdown
Member

I think this PR contains wrong changes like JDBC*. Is it correct?

@corneadoug

Copy link
Copy Markdown
Contributor

@rajarajan-g How do you handle if another user with same notebook name decides to gives you read ability on his notebook?

@ghost

Copy link
Copy Markdown
Author

@jongyoul : Thanks for noticing that. I didn't make those changes. Those changes seems to come from another commit. Need to check that

@ghost

Copy link
Copy Markdown
Author

@corneadoug : Nice catch. I didn't check that scenario. Will let you once, i figure out a way.
Let me check the current implementation too.

@corneadoug

Copy link
Copy Markdown
Contributor

Well the problem here is that there is really nothing you can do, you are not going to change your Notebook name, or somebody else notebook name because both name are the same

@ghost

Copy link
Copy Markdown
Author

@jongyoul : Hey i was quite busy for sometime.

I was trying to resolve the commit issues that you have pointed out.

but while during rebase i am getting below error

git rebase master
First, rewinding head to replay your work on top of it...
Applying: Code changes for ZEPPELIN-796
Using index info to reconstruct a base tree...
M zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
M zeppelin-web/src/components/noteName-create/notename.controller.js
M zeppelin-web/src/components/noteName-import/note-import-dialog.html
M zeppelin-web/src/components/noteName-import/notenameImport.controller.js
M zeppelin-web/src/components/websocketEvents/websocketEvents.factory.js
<stdin>:18: trailing whitespace.
* @throws DuplicateNameException <stdin>:40: trailing whitespace.
* @throws DuplicateNameException <stdin>:98: trailing whitespace.
* @throws DuplicateNameException <stdin>:175: trailing whitespace.
<stdin>:187: trailing whitespace.
conn.send(serializeMessage(new Message(OP.ERROR_DIALOG).put("info", warning: squelched 21 whitespace errors
warning: 26 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging zeppelin-web/src/components/websocketEvents/websocketEvents.factory.js
CONFLICT (content): Merge conflict in zeppelin-web/src/components/websocketEvents/websocketEvents.factory.js
Auto-merging zeppelin-web/src/components/noteName-import/notenameImport.controller.js
CONFLICT (content): Merge conflict in zeppelin-web/src/components/noteName-import/notenameImport.controller.js
Auto-merging zeppelin-web/src/components/noteName-import/note-import-dialog.html
Auto-merging zeppelin-web/src/components/noteName-create/notename.controller.js
CONFLICT (content): Merge conflict in zeppelin-web/src/components/noteName-create/notename.controller.js
Auto-merging zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
Failed to merge in the changes.
Patch failed at 0001 Code changes for ZEPPELIN-796
The copy of the patch that failed is found in:
/home/rajarajang/Workspace/stsWorksapce/zeppelin/.git/rebase-apply/patch
When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Even though after i resolve the conflicts, again it shows conflict again and not able to remove that unnecessary commit that you pointed out. Please help me out on resolving that issue

Please let me know if anyone has faced this issue and also please let me if there is a way to resolve this issue.

@ghost

Copy link
Copy Markdown
Author

@corneadoug : If a user creates a note with same name, it would do no good to that user itself as it confuses him.

For resolving the scenario, we can pick one of the below ways

  1. we should have one unique notebook name across the server.(Not quite a solution)
  2. we can follow unique notebook name per user in server.
    In case if a note is shared by the creator with other users, an extra information should be available to user like below in home page of notebook

NoteName(created by user1)

@ghostghost changed the title [Zeppelin 796] [WIP] Duplicated notebook names should not be allowed[Zeppelin 796] Duplicated notebook names should not be allowedOct 17, 2016
@ghost

Copy link
Copy Markdown
Author

Hi All,
An unknown commit got merged into this PR. I just had this issue while rebasing. I am going to open new PR for the same.So, Closing this PR . Please continue the discussion in the new PR #1536

@ghostghost closed this Oct 18, 2016
This pull request was closed.
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

@khalidhuseynov@jongyoul@corneadoug@felixcheung@raja-imaginea