[ZEPPELIN-808] Changed Permissions on Notebooks do not provide helpful error to user

[kavinkumarks]

What is this PR for?

This is about showing information to the user when there are errors on running paragraphs eg. there could be permission related issue with notebook.

What type of PR is it?

Improvement

Todos

NA

What is the Jira issue?

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

How should this be tested?

• Create a notebook and change the file system permission for the notebook folder to have system level write permission to a different user.For eg. if you are running your local zeppelin server with id [USERNAME], then change the file system permission for one of your notebooks created with the former username to different one eg. ROOT user who will only have the write permission
• Try to run all the paragraphs or any individual paragraph for the notebook
• The information as shown in the screenshot should be displayed and the dialog could be closed by the 'Close' button.

Screenshots (if appropriate)

Questions:

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

[kavinkumarks]

Could someone please review this?

Thanks,
Kavin
MailTo: [email protected]

[khalidhuseynov]

@kavinkumarks thanks for contribution. the idea of propagating error to the user is a good one and definitely improves users experience. However, in your implementation you basically propagate all kinds of errors (on top level catch Exception) with the same message to the user and user still not sure what the problem is. I think we need more fine grained approach here and let every method/feature implementor to be able to send ERROR_INFO message on his own catch statement for say PROPAGATE_USER_EXCEPTION with the message specific to error and throw other kinds of errors up to the logs. does that makes sense?

[kavinkumarks]

@khalidhuseynov thanks for sharing the inputs on this.Basically I like your idea of propagating the specific error to the user instead of the generic error message so the user would be more specific about the issue.If we need to handle other errors we can think about them in the future if there is need.

Here is my thought to the approach.I will change org.apache.zeppelin.notebook.repo.VFSNotebookRepo.save(Note, AuthenticationInfo) to throw the specific exception with the error message and handle the exception in org.apache.zeppelin.socket.NotebookServer.runParagraph(NotebookSocket, HashSet, Notebook, Message) and then broadcast the error message to the user.

Thanks,
Kavin
MailTo: [email protected]

[kavinkumarks]

@khalidhuseynov I have made the changes and committed them.I have handled the exception in runParagraph() instead of throwing custom exception from VFSNotebookRepo class since the exception need to be handled i.e FileSystemException (subclass of IOException) is already handled in different ways for other scenarios and in different implementations of NotebookRepo.

Thanks,
Kavin
MailTo: [email protected]

[khalidhuseynov]

@kavinkumarks this looks better and tested locally, works as expected. few things:

• would be nice to update screenshot above
• CI build fails, could you take a look into that or re-trigger it?

[kavinkumarks]

@khalidhuseynov thanks for checking the changes.I have updated the screenshot and triggered the CI build again.

Thanks,
Kavin
MailTo: [email protected]

[kavinkumarks]

Reopening to trigger the CI build.

[kavinkumarks]

The CI build is green.

[khalidhuseynov]

LGTM

[kavinkumarks]

Could someone else review and merge this?

Thanks,
Kavin
MailTo: [email protected]

[corneadoug]

@kavinkumarks I've tested this branch, but don't get the expected result when I follow the How should this be tested section.

I'm getting:

which is shown after receiving: AUTH_INFO

[kavinkumarks]

@corneadoug Sorry, I wasn't clear about the steps given in the PR.This was about the permission issue with the actual notebook file system and not with the permissions feature in the zeppelin UI.I have updated the PR description.

Please let me know if you have any questions.

Thanks,
Kavin
MailTo: [email protected]

[corneadoug]

Tested with the new instructions.
But, while I do get the error message, it is still running the query

[kavinkumarks]

@corneadoug Made changes to exit the paragraph execution when there are errors with the notebook file system.

Thanks,
Kavin
MailTo: [email protected]

[kavinkumarks]

Reopening to trigger the CI build.

[kavinkumarks]

The CI build is green.

[corneadoug]

@kavinkumarks This look like a good start, but is it the only case we wan't to handle here?

Maybe it's because of the way we test this PR, but if I change the system notebook permission to read only (chmod 444), I can't run it, but everything I do is still there after page refresh.

If I restart zeppelin-server, the notebook isn't there anymore.

[kavinkumarks]

@corneadoug I checked the various combinations of file permissions for the notebook folder eg. permissions 444,400. The issue that you reported above that the notebook is not listed on restarting the zeppelin server is due to the below error in the logs on restarting the server,

ERROR [2016-09-25 23:17:04,767] ({main} VFSNotebookRepo.java[list]:144) - Can't read note file:///home/kavink/git/zeppelin/notebook/2BWTQ6UDY java.io.IOException: file:///home/kavink/git/zeppelin/notebook/2BWTQ6UDY/note.json not found at org.apache.zeppelin.notebook.repo.VFSNotebookRepo.getNote(VFSNotebookRepo.java:158) at org.apache.zeppelin.notebook.repo.VFSNotebookRepo.getNoteInfo(VFSNotebookRepo.java:194) at org.apache.zeppelin.notebook.repo.VFSNotebookRepo.list(VFSNotebookRepo.java:139) at org.apache.zeppelin.notebook.repo.NotebookRepoSync.list(NotebookRepoSync.java:124) at org.apache.zeppelin.notebook.Notebook.loadAllNotes(Notebook.java:461) at org.apache.zeppelin.notebook.Notebook.<init>(Notebook.java:123) at org.apache.zeppelin.server.ZeppelinServer.<init>(ZeppelinServer.java:97) at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423)

The notebook folder should have both read and execute permission to list it and this is being already caught in the logs.I think that we aren't missing anything here.Please let me know your thoughts.

Thanks,
Kavin
MailTo: [email protected]

[kavinkumarks]

Reopening to trigger the CI build.

[corneadoug]

LGTM

[kavinkumarks]

The CI build is green.

@corneadoug thanks for the update! Could you please take care of merging this?

Thanks,
Kavin
MailTo: [email protected]

[corneadoug]

Merging if there is no more discussions