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 all 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
16 changes: 11 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,23 @@ 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) {
// 'result' part of Note consists of exception, instead of actual interpreter results
logger.warn("Paragraph " + srcParagraph.getId() + " has a result with exception. "
+ e.getMessage());
}

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);
Copy link
Member

@Leemoonsoo Leemoonsoo May 31, 2016

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.

Copy link
Contributor Author

@swkimme swkimme May 31, 2016

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 ?

Copy link
Member

@Leemoonsoo Leemoonsoo Jun 15, 2016

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