From 47d693520d9922286f92d0c55ce03f069f10391e Mon Sep 17 00:00:00 2001 From: Jongyoul Lee Date: Fri, 27 Apr 2018 14:45:18 +0900 Subject: [PATCH 1/5] Make `InterpreterJob` a public inner class to instantiate it outside of the class Set the type of `results` to InterpreterResult when exception occurred --- .../remote/RemoteInterpreterServer.java | 8 +- .../org/apache/zeppelin/scheduler/Job.java | 4 +- .../apache/zeppelin/scheduler/JobTest.java | 84 +++++++++++++++++++ 3 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.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 401be36b28c..e4db4696ce1 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 @@ -513,7 +513,8 @@ public void onStatusChange(Job job, Status before, Status after) { } } - 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; @@ -521,7 +522,7 @@ class InterpretJob extends Job { private Map infos; private Object results; - InterpretJob( + public InterpretJob( String jobId, String jobName, JobListener listener, @@ -592,7 +593,8 @@ public void onPostExecute(String script) { } @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); 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 579b60441a3..e687c4c7556 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.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; @@ -200,7 +202,7 @@ private synchronized void completeWithSuccess(Object result) { } private synchronized void completeWithError(Throwable error) { - setResult(error.getMessage()); + setResult(new InterpreterResult(Code.ERROR, error.getMessage())); setException(error); dateFinished = new Date(); } 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 00000000000..c9c8087e033 --- /dev/null +++ b/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java @@ -0,0 +1,84 @@ +/* + * 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); + assertEquals(((InterpreterResult)failedResult).message().get(0).getData(), failedMessage); + } +} From 1631e600742d6efa0356c1d19798d52e1b9eb40b Mon Sep 17 00:00:00 2001 From: Jongyoul Lee Date: Fri, 27 Apr 2018 14:53:29 +0900 Subject: [PATCH 2/5] Fix style --- .../src/test/java/org/apache/zeppelin/scheduler/JobTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index c9c8087e033..89a80f5ce62 100644 --- a/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java +++ b/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java @@ -79,6 +79,6 @@ public void testErrorCase() throws Throwable { Object failedResult = spyInterpretJob.getReturn(); assertTrue(failedResult instanceof InterpreterResult); - assertEquals(((InterpreterResult)failedResult).message().get(0).getData(), failedMessage); + assertEquals(((InterpreterResult) failedResult).message().get(0).getData(), failedMessage); } } From d47baccb44bceee35679d82ed43202398c495b4a Mon Sep 17 00:00:00 2001 From: Jongyoul Lee Date: Sun, 29 Apr 2018 01:51:30 +0900 Subject: [PATCH 3/5] Add getting stack trace logic when error occurs --- .../src/main/java/org/apache/zeppelin/scheduler/Job.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 e687c4c7556..141ea93faff 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,7 @@ import java.util.Map; import org.apache.commons.lang.exception.ExceptionUtils; +import org.apache.zeppelin.interpreter.InterpreterException; import org.apache.zeppelin.interpreter.InterpreterResult; import org.apache.zeppelin.interpreter.InterpreterResult.Code; import org.slf4j.Logger; @@ -202,7 +203,11 @@ private synchronized void completeWithSuccess(Object result) { } private synchronized void completeWithError(Throwable error) { - setResult(new InterpreterResult(Code.ERROR, error.getMessage())); + // TODO(jl): Remove this trick. Most of error are covered by InterpreterException + if (error instanceof InterpreterException && null != error.getCause()) { + error = error.getCause(); + } + setResult(new InterpreterResult(Code.ERROR, getStack(error))); setException(error); dateFinished = new Date(); } From 039f6a0469bc133c0e615a0dcd1316eb404dbf35 Mon Sep 17 00:00:00 2001 From: Jongyoul Lee Date: Mon, 30 Apr 2018 08:23:13 +0900 Subject: [PATCH 4/5] Fix JobTests --- .../src/test/java/org/apache/zeppelin/scheduler/JobTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index 89a80f5ce62..ea80a14a096 100644 --- a/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java +++ b/zeppelin-interpreter/src/test/java/org/apache/zeppelin/scheduler/JobTest.java @@ -79,6 +79,7 @@ public void testErrorCase() throws Throwable { Object failedResult = spyInterpretJob.getReturn(); assertTrue(failedResult instanceof InterpreterResult); - assertEquals(((InterpreterResult) failedResult).message().get(0).getData(), failedMessage); + assertTrue( + ((InterpreterResult) failedResult).message().get(0).getData().contains(failedMessage)); } } From c0bdb1bedf49f71a74f90731eefc0b62c5966731 Mon Sep 17 00:00:00 2001 From: Jongyoul Lee Date: Mon, 30 Apr 2018 14:57:07 +0900 Subject: [PATCH 5/5] Revert not to unwrap InterpreterException because found it's not propagated to Server from Interpreter --- .../src/main/java/org/apache/zeppelin/scheduler/Job.java | 5 ----- 1 file changed, 5 deletions(-) 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 141ea93faff..8e5c823349f 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,7 +22,6 @@ import java.util.Map; import org.apache.commons.lang.exception.ExceptionUtils; -import org.apache.zeppelin.interpreter.InterpreterException; import org.apache.zeppelin.interpreter.InterpreterResult; import org.apache.zeppelin.interpreter.InterpreterResult.Code; import org.slf4j.Logger; @@ -203,10 +202,6 @@ private synchronized void completeWithSuccess(Object result) { } private synchronized void completeWithError(Throwable error) { - // TODO(jl): Remove this trick. Most of error are covered by InterpreterException - if (error instanceof InterpreterException && null != error.getCause()) { - error = error.getCause(); - } setResult(new InterpreterResult(Code.ERROR, getStack(error))); setException(error); dateFinished = new Date();