Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
swkimme wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f8620cf
[ZEPPELIN-905] fix failed notebook import bug
swkimme May 31, 2016
91da565
[ZEPPELIN-905] add test
swkimme May 31, 2016
32c9172
fix test, better implementation
swkimme May 31, 2016
04e777c
revert implementation
swkimme Jun 1, 2016
27ff2ef
trigger CI build
swkimme Jun 1, 2016
e51889d
log exception
swkimme Jun 3, 2016
3b16f02
log info -> warn, add message
swkimme Jun 3, 2016
571e8fc
stylish code
swkimme Jun 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,17 +175,19 @@ public void addCloneParagraph(Paragraph srcParagraph) {
Map<String, Object> config = new HashMap<>(srcParagraph.getConfig());
Map<String, Object> param = new HashMap<>(srcParagraph.settings.getParams());
Map<String, Input> form = new HashMap<>(srcParagraph.settings.getForms());
Gson gson = new Gson();
InterpreterResult result = gson.fromJson(
gson.toJson(srcParagraph.getReturn()),
InterpreterResult.class);

newParagraph.setConfig(config);
newParagraph.settings.setParams(param);
newParagraph.settings.setForms(form);
newParagraph.setText(srcParagraph.getText());
newParagraph.setTitle(srcParagraph.getTitle());
newParagraph.setReturn(result, null);

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

@bzz bzz Jun 1, 2016 •
edited
Loading

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.

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?

@felixcheung felixcheung Jun 2, 2016

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.

and, I think we should log the exception


synchronized (paragraphs) {
paragraphs.add(newParagraph);
Expand Down
25 changes: 25 additions & 0 deletions zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,31 @@ public void testAutoRestartInterpreterAfterSchedule() throws InterruptedExceptio
notebook.refreshCron(note.id());
}

@Test
public void testExportAndImportNote() throws IOException, CloneNotSupportedException,
InterruptedException {
Note note = notebook.createNote();
note.getNoteReplLoader().setInterpreters(factory.getDefaultInterpreterSettingList());

final Paragraph p = note.addParagraph();
String simpleText = "hello world";
p.setText(simpleText);

@Leemoonsoo Leemoonsoo May 31, 2016

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.

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.

@swkimme swkimme May 31, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@Leemoonsoo Leemoonsoo Jun 15, 2016

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.

@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.


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

String exportedNoteJson = notebook.exportNote(note.getId());

Note importedNote = notebook.importNote(exportedNoteJson, "Title");

Paragraph p2 = importedNote.getParagraphs().get(0);

// Test
assertEquals(p.getId(), p2.getId());
assertEquals(p.text, p2.text);
assertEquals(p.getResult().message(), p2.getResult().message());
}

@Test
public void testCloneNote() throws IOException, CloneNotSupportedException,
InterruptedException {
Expand Down