Repository: zeppelin Updated Branches: refs/heads/branch-0.8 20384bfba -> 29cf9186c
ZEPPELIN-3432 Fix results object when throwing exception while running job ### What is this PR for? Handling the issue that `InterpretJob.jobRun()` throws an `InterpreterException` ### What type of PR is it? [Bug Fix] ### Todos * [x] - Change the logic to create `InterpreterResult` when throwing `InterpreterException` ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3432 ### How should this be tested? 1. Enable `zeppelin.spark.sql.stacktrace` option 2. %sql select invalid query ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Jongyoul Lee <jongy...@gmail.com> Closes #2950 from jongyoul/ZEPPELIN-3432 and squashes the following commits: c0bdb1bed [Jongyoul Lee] Revert not to unwrap InterpreterException because found it's not propagated to Server from Interpreter 039f6a046 [Jongyoul Lee] Fix JobTests d47baccb4 [Jongyoul Lee] Add getting stack trace logic when error occurs 1631e6007 [Jongyoul Lee] Fix style 47d693520 [Jongyoul Lee] Make `InterpreterJob` a public inner class to instantiate it outside of the class Set the type of `results` to InterpreterResult when exception occurred (cherry picked from commit e503623de6c3c9406e33991dab8e3872c7105092) Signed-off-by: Jongyoul Lee <jongy...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/29cf9186 Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/29cf9186 Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/29cf9186 Branch: refs/heads/branch-0.8 Commit: 29cf9186cb1e439c1fbb9463141c46d7b45cbe5e Parents: 20384bf Author: Jongyoul Lee <jongy...@gmail.com> Authored: Mon Apr 30 14:57:07 2018 +0900 Committer: Jongyoul Lee <jongy...@apache.org> Committed: Wed May 2 13:04:44 2018 +0900 ---------------------------------------------------------------------- .../remote/RemoteInterpreterServer.java | 6 +- .../java/org/apache/zeppelin/scheduler/Job.java | 4 +- .../org/apache/zeppelin/scheduler/JobTest.java | 85 ++++++++++++++++++++ 3 files changed, 92 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/29cf9186/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java ---------------------------------------------------------------------- diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java index 591037a..be3cc36 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java @@ -517,7 +517,8 @@ public class RemoteInterpreterServer extends Thread } } - class InterpretJob extends Job { + // TODO(jl): Need to extract this class from RemoteInterpreterServer to test it + public static class InterpretJob extends Job { private Interpreter interpreter; private String script; @@ -596,7 +597,8 @@ public class RemoteInterpreterServer extends Thread } @Override - protected Object jobRun() throws Throwable { + // TODO(jl): need to redesign this class + public Object jobRun() throws Throwable { ClassLoader currentThreadContextClassloader = Thread.currentThread().getContextClassLoader(); try { InterpreterContext.set(context); http://git-wip-us.apache.org/repos/asf/zeppelin/blob/29cf9186/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java ---------------------------------------------------------------------- diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java index 8e25f7b..2a9dd63 100644 --- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java +++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java @@ -22,6 +22,8 @@ import java.util.Date; import java.util.Map; import org.apache.commons.lang.exception.ExceptionUtils; +import org.apache.zeppelin.interpreter.InterpreterResult; +import org.apache.zeppelin.interpreter.InterpreterResult.Code; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -203,7 +205,7 @@ public abstract class Job { } private synchronized void completeWithError(Throwable error) { - setResult(error.getMessage()); + setResult(new InterpreterResult(Code.ERROR, getStack(error))); setException(error); dateFinished = new Date(); } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/29cf9186/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java ---------------------------------------------------------------------- diff --git a/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java b/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java new file mode 100644 index 0000000..ea80a14 --- /dev/null +++ b/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zeppelin.scheduler; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.spy; + +import org.apache.zeppelin.interpreter.Interpreter; +import org.apache.zeppelin.interpreter.InterpreterContext; +import org.apache.zeppelin.interpreter.InterpreterException; +import org.apache.zeppelin.interpreter.InterpreterResult; +import org.apache.zeppelin.interpreter.InterpreterResult.Code; +import org.apache.zeppelin.interpreter.remote.RemoteInterpreterServer.InterpretJob; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class JobTest { + + @Mock private JobListener mockJobListener; + @Mock private Interpreter mockInterpreter; + @Mock private InterpreterContext mockInterpreterContext; + private InterpretJob spyInterpretJob; + + @Before + public void setUp() throws Exception { + InterpretJob interpretJob = + new InterpretJob( + "jobid", + "jobName", + mockJobListener, + 10000, + mockInterpreter, + "script", + mockInterpreterContext); + spyInterpretJob = spy(interpretJob); + } + + @Test + public void testNormalCase() throws Throwable { + + InterpreterResult successInterpreterResult = + new InterpreterResult(Code.SUCCESS, "success result"); + doReturn(successInterpreterResult).when(spyInterpretJob).jobRun(); + + spyInterpretJob.run(); + + assertEquals(successInterpreterResult, spyInterpretJob.getReturn()); + } + + @Test + public void testErrorCase() throws Throwable { + String failedMessage = "failed message"; + InterpreterException interpreterException = new InterpreterException(failedMessage); + doThrow(interpreterException).when(spyInterpretJob).jobRun(); + + spyInterpretJob.run(); + + Object failedResult = spyInterpretJob.getReturn(); + assertTrue(failedResult instanceof InterpreterResult); + assertTrue( + ((InterpreterResult) failedResult).message().get(0).getData().contains(failedMessage)); + } +}
