Skip to content

Conversation

@minahlee
Copy link
Member

@minahlee minahlee commented Jun 18, 2016

What is this PR for?

This PR is fixing import/clone notebook with error result. This PR adds test based on #933.

Note: This issue is one of the blockers of 0.6.0 release so should be merged into branch-0.6 before release.

What type of PR is it?

Bug Fix

What is the Jira issue?

ZEPPELIN-905

How should this be tested?

When you try to import or clone notebook with error result, it should work.

Questions:

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

@minahlee
Copy link
Member Author

@swkimme @Leemoonsoo Ready for review

@bzz
Copy link
Member

bzz commented Jun 20, 2016

Looks good to me

2 CI failures seems un-related (not sure if it's been taken care of somewhere)

1

08:03:25,846 ERROR org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess:349 - Remote interpreter process not started
java.lang.NullPointerException
    at org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess.updateRemoteAngularObject(RemoteInterpreterProcess.java:344)
    at org.apache.zeppelin.interpreter.remote.RemoteAngularObject.set(RemoteAngularObject.java:48)
...
Results :

Failed tests: 
  ZeppelinSparkClusterTest.sparkRTest:104 expected:<FINISHED> but was:<ERROR>

2

Failed tests: 
  ParagraphActionsIT.testTitleButton:355 After Hide Title : The title field contains
Expected: ""
     but: was "Untitled"

Tests in error: 
  ParagraphActionsIT.testRemoveButton:153 » NoSuchElement Unable to locate eleme...
  ParagraphActionsIT.testCreateNewButton:86 » NoSuchElement Unable to locate ele...

Tests run: 15, Failures: 1, Errors: 2, Skipped: 0

newParagraph.setReturn(result, null);
} catch (Exception e) {
// 'result' part of Note consists of exception, instead of actual interpreter results
logger.warn("Paragraph " + srcParagraph.getId() + " has a result with exception. "
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a logger.error and re-throw the exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my understanding, if we re-throw error, import/clone notebook with Object result(for example String) other than InterpreterResult will fail as current master is.

Copy link
Member

Choose a reason for hiding this comment

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

@felixcheung good question, but I think this just happens to be an exception here and it does not represent an error condition in our logic, as it's fine for current notebook structure to have paragraph with an exception stack trace in the output.

@bzz
Copy link
Member

bzz commented Jun 21, 2016

I think we should merge it ASAP as per ZEPPELIN-889 this blocks us from makeing a release.

Any 👍 or 👎 ?

@AhyoungRyu
Copy link
Contributor

@bzz Let's merge this one 👍

@Leemoonsoo
Copy link
Member

LGTM

@minahlee
Copy link
Member Author

Merging it into master and branch-0.6 if there is no more discussion

@asfgit asfgit closed this in 50db175 Jun 23, 2016
asfgit pushed a commit that referenced this pull request Jun 23, 2016
### What is this PR for?
This PR is fixing import/clone notebook with error result. This PR adds test based on #933.
> Note: This issue is one of the [blockers](https://issues.apache.org/jira/browse/ZEPPELIN-889) of 0.6.0 release so should be merged into branch-0.6 before release.

### What type of PR is it?
Bug Fix

### What is the Jira issue?
[ZEPPELIN-905](https://issues.apache.org/jira/browse/ZEPPELIN-905)

### How should this be tested?
When you try to import or clone notebook with error result, it should work.

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Kevin Kim <[email protected]>
Author: Mina Lee <[email protected]>

Closes #1043 from minahlee/ZEPPELIN-905 and squashes the following commits:

69b8c02 [Mina Lee] Add test for clone notebook with String type result
e7af919 [Kevin Kim] stylish code
7bf5d01 [Kevin Kim] log info -> warn, add message
d4f6699 [Kevin Kim] log exception
32949bc [Kevin Kim] trigger CI build
803e08a [Kevin Kim] revert implementation
c13293f [Kevin Kim] fix test, better implementation
1e45a9e [Kevin Kim] [ZEPPELIN-905] add test
a4188be [Kevin Kim] [ZEPPELIN-905] fix failed notebook import bug

(cherry picked from commit 50db175)
Signed-off-by: Mina Lee <[email protected]>
minahlee added a commit to minahlee/zeppelin that referenced this pull request Jun 23, 2016
@minahlee minahlee deleted the ZEPPELIN-905 branch June 23, 2016 05:45
asfgit pushed a commit that referenced this pull request Jun 23, 2016
### What is this PR for?
Hotfix for compile error of master, branch-0.6 after merging #1043

Author: Mina Lee <[email protected]>

Closes #1070 from minahlee/hotfix/compile_error and squashes the following commits:

b0d5a6b [Mina Lee] Fix compile error after merging #1043
asfgit pushed a commit that referenced this pull request Jun 23, 2016
### What is this PR for?
Hotfix for compile error of master, branch-0.6 after merging #1043

Author: Mina Lee <[email protected]>

Closes #1070 from minahlee/hotfix/compile_error and squashes the following commits:

b0d5a6b [Mina Lee] Fix compile error after merging #1043

(cherry picked from commit 83602f5)
Signed-off-by: Mina Lee <[email protected]>
asfgit pushed a commit that referenced this pull request Jun 26, 2016
Closes #276 (fixed by #752)
Closes #381 (fixed by #378)
Closes #933 (fixed by #1043)
Sign up for free to 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.

6 participants