[ZEPPELIN-1612] Fix NPE when initializing Notebook #1590
Conversation
There was a problem hiding this comment.
Just wonder if that means that notes are always load as anonymous irregarding Zeppelin mode, because we do not have such permissions yet?
Curious, as I would expect that we want to load all notes for a specific user, in case auth is enabled in Zeppelin.
Other then that - looks great to me! Huge 👍 for tests
| note.persist(anonymous); | ||
| } catch (IOException fe) { | ||
| logger.warn("Failed to create note and paragraph. Safe to return, other tests might be failing as well", fe); | ||
| return; |
There was a problem hiding this comment.
The error message above implies that it is intended as failing condition for this test, but return will actually make test pass.
What was the intention? I think it's fine to explicitly fail the test if some pre-condition fail
There was a problem hiding this comment.
Actually if precondition fails it means the problem is in file saving routine and not related to testing this certain function. In this case we have option of being 1) strict and fail this test as well, or 2) being less strict since failing precondition in this case means that note.persist is failing and probably test for that function should be failing as well. I chose here second option initially, but it makes sense to make it more strict; i'll change it then.
|
@bzz that function |
|
Got it! Thank you for kind explanation. CI failure looks not relevant for the change set. Looks great to me, merging to master if there is no further discussion. |
### What is this PR for? Sometimes Zeppelin wasn't able to start because of empty subject when initializing Notebook class, more details in issue. ### What type of PR is it? Bug Fix ### Todos * [x] - add anonymous subject * [x] - add test ### What is the Jira issue? [ZEPPELIN-1612](https://issues.apache.org/jira/browse/ZEPPELIN-1612) ### How should this be tested? * added test passing and no relevant CI failures * also can be starting Zeppelin in anonymous mode when you have notes with angular objects ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Khalid Huseynov <[email protected]> Closes apache#1590 from khalidhuseynov/fix/loadAllNotes-npe and squashes the following commits: 0d21f78 [Khalid Huseynov] strict test passing condition 8da069b [Khalid Huseynov] add test 3dc0a8b [Khalid Huseynov] substitute null with anonymous subject
### What is this PR for? Sometimes Zeppelin wasn't able to start because of empty subject when initializing Notebook class, more details in issue. ### What type of PR is it? Bug Fix ### Todos * [x] - add anonymous subject * [x] - add test ### What is the Jira issue? [ZEPPELIN-1612](https://issues.apache.org/jira/browse/ZEPPELIN-1612) ### How should this be tested? * added test passing and no relevant CI failures * also can be starting Zeppelin in anonymous mode when you have notes with angular objects ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Khalid Huseynov <[email protected]> Closes apache#1590 from khalidhuseynov/fix/loadAllNotes-npe and squashes the following commits: 0d21f78 [Khalid Huseynov] strict test passing condition 8da069b [Khalid Huseynov] add test 3dc0a8b [Khalid Huseynov] substitute null with anonymous subject
2 participants
What is this PR for?
Sometimes Zeppelin wasn't able to start because of empty subject when initializing Notebook class, more details in issue.
What type of PR is it?
Bug Fix
Todos
What is the Jira issue?
ZEPPELIN-1612
How should this be tested?
Screenshots (if appropriate)
Questions: