[ZEPPELIN-905] fix failed notebook import bug #933

swkimme · 2016-05-31T03:13:49Z

What is this PR for?

When notebook has a result with Exceptions, it always fail to import. This PR fix it.

What type of PR is it?

Bug Fix

Todos

- Fix this bug by ignore the result with error.

What is the Jira issue?

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

How should this be tested?

Create notebook with error, then try to import.

Questions:

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

Leemoonsoo · 2016-05-31T04:36:44Z

Looks good to me.
How about adding an unittest for it in NotebookTest.java? Let me know if you need help!

swkimme · 2016-05-31T05:14:28Z

@Leemoonsoo
I've added some test!

Leemoonsoo · 2016-05-31T05:32:02Z

zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java

+    note.getNoteReplLoader().setInterpreters(factory.getDefaultInterpreterSettingList());
+
+    final Paragraph p = note.addParagraph();
+    String simpleText = "hello world";


How about make p.getReturn() returns non InterpreterResult type object here? Then this test can simulate exception when doing InterpreterResult result = gson.fromJson(resultJson, InterpreterResult.class);

I think easiest way to set arbitrary object to Job.result is making Job.setResult() public.

@Leemoonsoo I cannot agree that's the best way to do that..
What if we change type of Job.result into InterpreterResult ?

@swkimme
I was trying to say, the test doesn't test import note when p.getReturn() is String.
However this unittest is still good, too. Anyway we didn't have test for import/export function.

About changing Job.result into InterpreterResult, we'll need to consider import note.json created from previous version of Zeppelin. They'll still have Job.result with string type.

swkimme · 2016-05-31T07:54:22Z

@Leemoonsoo Do you know why the CI test failed btw?

prabhjyotsingh · 2016-05-31T09:43:57Z

@swkimme I think this is failing because you are trying to get paragraph result i.e. p.getResult() or p2.getResult() without running any of the paragraph.

try having

note.runAll();
while(p.isTerminated()==false || p.getResult()==null) Thread.yield();

importedNote.runAll();
while(p2.isTerminated()==false || p2.getResult()==null) Thread.yield();

bzz · 2016-06-01T09:37:43Z

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

+      String resultJson = gson.toJson(srcParagraph.getReturn());
+      InterpreterResult result = gson.fromJson(resultJson, InterpreterResult.class);
+      newParagraph.setReturn(result, null);
+    } catch (Exception e) { /* ignore */ }


A nitpick, but may be we could leave some clues for the person who is going to read it next time.

Instead of stating the fact that we are ignoring exception, we could write the reason or case when\why it happens to be OK to do so i.e

// ignore case when 'result' part of Note consists of exception, instead of actual interpreter results

would make sense, how do you think?

and, I think we should log the exception

swkimme · 2016-06-03T03:49:28Z

@bzz @felixcheung Good idea, updated log and comment!

felixcheung · 2016-06-03T05:04:40Z

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

-    } catch (Exception e) { /* ignore */ }
+    } catch (Exception e) {
+      // 'result' part of Note consists of exception, instead of actual interpreter results
+      logger.info("Paragraph " + srcParagraph.getId() + " has a result with exception.");


could you ether logger.error() or getStackTrace like here?

The exception and message is quite clear so I don't think we do not need trace, then I'm adding the error message!

bzz · 2016-06-15T07:18:10Z

do you think it's ready to be merged now? I belive there was some feedback from @Leemoonsoo as well

As for CI failure - it seems like flaky ZeppelinIT.testSparkInterpreterDependencyLoading:243 so would you mind rebasing on latest master please?

bzz · 2016-06-16T15:40:27Z

@swkimme ping on the status of this patch

### 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

### 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]>

[ZEPPELIN-905] fix failed notebook import bug

f8620cf

[ZEPPELIN-905] add test

91da565

Leemoonsoo reviewed May 31, 2016
View reviewed changes

swkimme added 3 commits May 31, 2016 19:23

fix test, better implementation

32c9172

revert implementation

04e777c

trigger CI build

27ff2ef

bzz reviewed Jun 1, 2016
View reviewed changes

log exception

e51889d

felixcheung reviewed Jun 3, 2016
View reviewed changes

swkimme added 2 commits June 3, 2016 19:02

log info -> warn, add message

3b16f02

stylish code

571e8fc

This was referenced Jun 18, 2016

[WIP] Fix notebook clone bug #458

Closed

[ZEPPELIN-905] Fix import notebook with error result #1043

Closed

asfgit closed this in 0209bc3 Jun 26, 2016

[ZEPPELIN-905] fix failed notebook import bug #933

[ZEPPELIN-905] fix failed notebook import bug #933

Uh oh!

Conversation

swkimme commented May 31, 2016

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

Leemoonsoo commented May 31, 2016

Uh oh!

swkimme commented May 31, 2016

Uh oh!

Leemoonsoo May 31, 2016

Choose a reason for hiding this comment

Uh oh!

swkimme May 31, 2016

Choose a reason for hiding this comment

Uh oh!

Leemoonsoo Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

swkimme commented May 31, 2016

Uh oh!

prabhjyotsingh commented May 31, 2016

Uh oh!

bzz Jun 1, 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!

felixcheung Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

swkimme commented Jun 3, 2016

Uh oh!

felixcheung Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

swkimme Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

bzz commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bzz commented Jun 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bzz Jun 1, 2016 •

edited

Loading

bzz commented Jun 15, 2016 •

edited

Loading