Skip to content

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

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
minahlee wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4188be
[ZEPPELIN-905] fix failed notebook import bug
swkimme May 31, 2016
1e45a9e
[ZEPPELIN-905] add test
swkimme May 31, 2016
c13293f
fix test, better implementation
swkimme May 31, 2016
803e08a
revert implementation
swkimme Jun 1, 2016
32949bc
trigger CI build
swkimme Jun 1, 2016
d4f6699
log exception
swkimme Jun 3, 2016
7bf5d01
log info -> warn, add message
swkimme Jun 3, 2016
e7af919
stylish code
swkimme Jun 3, 2016
69b8c02
Add test for clone notebook with String type result
minahlee Jun 18, 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
2 changes: 1 addition & 1 deletion zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ public Date getDateFinished() {
return dateFinished;
}

protected void setResult(Object result) {
public void setResult(Object result) {
this.result = result;
}
}
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 @@ -207,17 +207,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. "
Copy link
Member

@felixcheung felixcheung Jun 20, 2016

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

@minahlee minahlee Jun 20, 2016

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

@bzz bzz Jun 21, 2016

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.

+ e.getMessage());
}

synchronized (paragraphs) {
paragraphs.add(newParagraph);
Expand Down
52 changes: 51 additions & 1 deletion 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 @@ -43,7 +43,6 @@
import org.apache.zeppelin.resource.ResourcePoolUtils;
import org.apache.zeppelin.scheduler.Job;
import org.apache.zeppelin.scheduler.Job.Status;
import org.apache.zeppelin.scheduler.JobListener;
import org.apache.zeppelin.scheduler.SchedulerFactory;
import org.apache.zeppelin.search.SearchService;
import org.apache.zeppelin.user.Credentials;
Expand Down Expand Up @@ -316,6 +315,33 @@ 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);

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 All @@ -338,6 +364,30 @@ public void testCloneNote() throws IOException, CloneNotSupportedException,
assertEquals(cp.getResult().message(), p.getResult().message());
}

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

final Paragraph p = note.addParagraph();
p.setText("hello world");
note.runAll();
while (p.isTerminated() == false || p.getResult() == null) {
Thread.yield();
}
// Force paragraph to have String type object
p.setResult("Exception");

Note cloneNote = notebook.cloneNote(note.getId(), "clone note with Exception result");
Paragraph cp = cloneNote.paragraphs.get(0);

// Keep same ParagraphID
assertEquals(cp.getId(), p.getId());
assertEquals(cp.text, p.text);
assertNull(cp.getResult());
}

@Test
public void testResourceRemovealOnParagraphNoteRemove() throws IOException {
Note note = notebook.createNote();
Expand Down