Repository: zeppelin Updated Branches: refs/heads/master 211e55d01 -> 40cfc5a40
ZEPPELIN-1225. Errors before the last shell command are ignored ### What is this PR for? The problem is that command "bash -c <shell scripts>" will always return 0 as long as the last line of shell script run correctly. e.g the following command will run correctly without any error message. ``` hello pwd ``` This PR will redirect stderr and stdout to the same place, and will display both the stderr and stdout to frontend just like what user see in the native shell terminal. So the output of above command will be as following ``` bash: hello: command not found /Users/jzhang/github/zeppelin ``` ### What type of PR is it? [Bug Fix] ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-1225 ### How should this be tested? Unit test is added and also manually verify it on zeppelin notebook. ### 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: Jeff Zhang <zjf...@apache.org> This patch had conflicts when merged, resolved by Committer: Lee moon soo <m...@apache.org> Closes #1215 from zjffdu/ZEPPELIN-1225 and squashes the following commits: aa87b66 [Jeff Zhang] should clear executors after shell execution is completed 0266c71 [Jeff Zhang] ZEPPELIN-1225. Errors before the last shell command are ignored Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/40cfc5a4 Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/40cfc5a4 Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/40cfc5a4 Branch: refs/heads/master Commit: 40cfc5a408de49214ab77d31273afde7188a4cea Parents: 211e55d Author: Jeff Zhang <zjf...@apache.org> Authored: Fri Jul 22 14:30:31 2016 +0800 Committer: Lee moon soo <m...@apache.org> Committed: Fri Jul 29 15:11:55 2016 +0900 ---------------------------------------------------------------------- .../apache/zeppelin/shell/ShellInterpreter.java | 20 +-- .../zeppelin/shell/ShellInterpreterTest.java | 137 +++++++++++-------- 2 files changed, 88 insertions(+), 69 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/40cfc5a4/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java ---------------------------------------------------------------------- diff --git a/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java b/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java index 9619de5..3e09b8b 100644 --- a/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java +++ b/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java @@ -24,6 +24,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; import org.apache.commons.exec.CommandLine; import org.apache.commons.exec.DefaultExecutor; @@ -50,7 +51,7 @@ public class ShellInterpreter extends Interpreter { private static final String TIMEOUT_PROPERTY = "shell.command.timeout.millisecs"; private final boolean isWindows = System.getProperty("os.name").startsWith("Windows"); private final String shell = isWindows ? "cmd /c" : "bash -c"; - private Map<String, DefaultExecutor> executors; + ConcurrentHashMap<String, DefaultExecutor> executors; public ShellInterpreter(Properties property) { super(property); @@ -59,7 +60,7 @@ public class ShellInterpreter extends Interpreter { @Override public void open() { LOGGER.info("Command timeout property: {}", getProperty(TIMEOUT_PROPERTY)); - executors = new HashMap<String, DefaultExecutor>(); + executors = new ConcurrentHashMap<String, DefaultExecutor>(); if (!StringUtils.isAnyEmpty(getProperty("zeppelin.shell.auth.type"))) { ShellSecurityImpl.createSecureConfiguration(getProperty(), shell); } @@ -73,7 +74,6 @@ public class ShellInterpreter extends Interpreter { public InterpreterResult interpret(String cmd, InterpreterContext contextInterpreter) { LOGGER.debug("Run shell command '" + cmd + "'"); OutputStream outStream = new ByteArrayOutputStream(); - OutputStream errStream = new ByteArrayOutputStream(); CommandLine cmdLine = CommandLine.parse(shell); // the Windows CMD shell doesn't handle multiline statements, @@ -86,7 +86,7 @@ public class ShellInterpreter extends Interpreter { try { DefaultExecutor executor = new DefaultExecutor(); - executor.setStreamHandler(new PumpStreamHandler(outStream, errStream)); + executor.setStreamHandler(new PumpStreamHandler(outStream, outStream)); executor.setWatchdog(new ExecuteWatchdog(Long.valueOf(getProperty(TIMEOUT_PROPERTY)))); executors.put(contextInterpreter.getParagraphId(), executor); int exitVal = executor.execute(cmdLine); @@ -97,7 +97,7 @@ public class ShellInterpreter extends Interpreter { int exitValue = e.getExitValue(); LOGGER.error("Can not run " + cmd, e); Code code = Code.ERROR; - String message = errStream.toString(); + String message = outStream.toString(); if (exitValue == 143) { code = Code.INCOMPLETE; message += "Paragraph received a SIGTERM.\n"; @@ -109,16 +109,16 @@ public class ShellInterpreter extends Interpreter { } catch (IOException e) { LOGGER.error("Can not run " + cmd, e); return new InterpreterResult(Code.ERROR, e.getMessage()); + } finally { + executors.remove(contextInterpreter.getParagraphId()); } } @Override public void cancel(InterpreterContext context) { - for (String paragraphId : executors.keySet()) { - if (paragraphId.equals(context.getParagraphId())) { - DefaultExecutor executor = executors.get(paragraphId); - executor.getWatchdog().destroyProcess(); - } + DefaultExecutor executor = executors.remove(context.getParagraphId()); + if (executor != null) { + executor.getWatchdog().destroyProcess(); } } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/40cfc5a4/shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java ---------------------------------------------------------------------- diff --git a/shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java b/shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java index cb96df7..acdb65c 100644 --- a/shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java +++ b/shell/src/test/java/org/apache/zeppelin/shell/ShellInterpreterTest.java @@ -1,59 +1,78 @@ -/* - * 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.shell; - -import static org.junit.Assert.assertEquals; - -import java.util.Properties; - -import org.apache.zeppelin.interpreter.InterpreterContext; -import org.apache.zeppelin.interpreter.InterpreterResult; -import org.apache.zeppelin.interpreter.InterpreterResult.Code; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -public class ShellInterpreterTest { - - private ShellInterpreter shell; - - @Before - public void setUp() throws Exception { - Properties p = new Properties(); - p.setProperty("shell.command.timeout.millisecs", "60000"); - shell = new ShellInterpreter(p); - } - - @After - public void tearDown() throws Exception { - } - - @Test - public void test() { - shell.open(); - InterpreterContext context = new InterpreterContext("", "1", "", "", null, null, null, null, null, null, null); - InterpreterResult result = new InterpreterResult(Code.ERROR); - if (System.getProperty("os.name").startsWith("Windows")) { - result = shell.interpret("dir", context); - } else { - result = shell.interpret("ls", context); - } - assertEquals(InterpreterResult.Code.SUCCESS, result.code()); - } - -} +/* + * 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.shell; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Properties; + +import org.apache.zeppelin.interpreter.InterpreterContext; +import org.apache.zeppelin.interpreter.InterpreterResult; +import org.apache.zeppelin.interpreter.InterpreterResult.Code; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class ShellInterpreterTest { + + private ShellInterpreter shell; + + @Before + public void setUp() throws Exception { + Properties p = new Properties(); + p.setProperty("shell.command.timeout.millisecs", "60000"); + shell = new ShellInterpreter(p); + } + + @After + public void tearDown() throws Exception { + } + + @Test + public void test() { + shell.open(); + InterpreterContext context = new InterpreterContext("", "1", "", "", null, null, null, null, null, null, null); + InterpreterResult result = new InterpreterResult(Code.ERROR); + if (System.getProperty("os.name").startsWith("Windows")) { + result = shell.interpret("dir", context); + } else { + result = shell.interpret("ls", context); + } + assertEquals(InterpreterResult.Code.SUCCESS, result.code()); + assertTrue(shell.executors.isEmpty()); + // it should be fine to cancel a statement that has been completed. + shell.cancel(context); + assertTrue(shell.executors.isEmpty()); + } + + @Test + public void testInvalidCommand(){ + shell.open(); + InterpreterContext context = new InterpreterContext("","1","","",null,null,null,null,null,null,null); + InterpreterResult result = new InterpreterResult(Code.ERROR); + if (System.getProperty("os.name").startsWith("Windows")) { + result = shell.interpret("invalid_command\ndir",context); + } else { + result = shell.interpret("invalid_command\nls",context); + } + assertEquals(InterpreterResult.Code.SUCCESS,result.code()); + assertTrue(result.message().contains("invalid_command")); + } + +}
