[zeppelin] branch master updated: [ZEPPELIN-4641]. Close interpreter in LazyOpenInterpreter when fail to open interpreter

[zj...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zjffdu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git


The following commit(s) were added to refs/heads/master by this push:
     new 381d9fd  [ZEPPELIN-4641]. Close interpreter in LazyOpenInterpreter when fail to open interpreter
381d9fd is described below

commit 381d9fdf139f48592266808c6349296da33c3ed1
Author: Jeff Zhang <zj...@apache.org>
AuthorDate: Tue Feb 25 23:10:02 2020 +0800

    [ZEPPELIN-4641]. Close interpreter in LazyOpenInterpreter when fail to open interpreter
    
    ### What is this PR for?
    Currently, resource may leak if interpreter fail to open, and reopen it again. e.g
    In flink interpreter, we will create a yarn app in open method, but may fail in the following initialization. Then if user open it again, he would create another yarn app, the first yarn app is leaked.
    
    This PR fix it via close interpreter in LazyOpenInterpreter. Besides that it fix one bug in `ProcessLauncher`, otherwise the CI will fail.
    
    ### What type of PR is it?
    [Improvement]
    
    ### Todos
    * [ ] - Task
    
    ### What is the Jira issue?
    * https://issues.apache.org/jira/browse/ZEPPELIN-4641
    
    ### How should this be tested?
    * Manually Tested
    
    ### 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 <zj...@apache.org>
    
    Closes #3660 from zjffdu/ZEPPELIN-4641 and squashes the following commits:
    
    44f1476d5 [Jeff Zhang] [ZEPPELIN-4641]. Close interpreter in LazyOpenInterpreter when fail to open interpreter
---
 rlang/src/main/java/org/apache/zeppelin/r/RInterpreter.java   |  2 +-
 .../org/apache/zeppelin/interpreter/LazyOpenInterpreter.java  | 11 +++++++++--
 .../org/apache/zeppelin/interpreter/util/ProcessLauncher.java |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/rlang/src/main/java/org/apache/zeppelin/r/RInterpreter.java b/rlang/src/main/java/org/apache/zeppelin/r/RInterpreter.java
index 3fd3c88..aecbab3 100644
--- a/rlang/src/main/java/org/apache/zeppelin/r/RInterpreter.java
+++ b/rlang/src/main/java/org/apache/zeppelin/r/RInterpreter.java
@@ -110,7 +110,7 @@ public class RInterpreter extends AbstractInterpreter {
       zeppelinR.open();
       LOGGER.info("ZeppelinR is opened successfully.");
     } catch (IOException e) {
-      throw new InterpreterException("Exception while opening SparkRInterpreter", e);
+      throw new InterpreterException("Exception while opening RInterpreter", e);
     }
 
     if (useKnitr) {
diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/LazyOpenInterpreter.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/LazyOpenInterpreter.java
index 7581e67..3e1e490 100644
--- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/LazyOpenInterpreter.java
+++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/LazyOpenInterpreter.java
@@ -66,8 +66,15 @@ public class LazyOpenInterpreter
 
     synchronized (intp) {
       if (opened == false) {
-        intp.open();
-        opened = true;
+        try {
+          intp.open();
+          opened = true;
+        } catch (Throwable e) {
+          // close interpreter to release resource,
+          // otherwise these resources may leak when open it again.
+          intp.close();
+          throw new InterpreterException(e);
+        }
       }
     }
   }
diff --git a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/ProcessLauncher.java b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/ProcessLauncher.java
index 3d37c65..85c1cfe 100644
--- a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/ProcessLauncher.java
+++ b/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/ProcessLauncher.java
@@ -153,7 +153,7 @@ public abstract class ProcessLauncher implements ExecuteResultHandler {
   }
 
   public void stop() {
-    if (watchdog != null) {
+    if (watchdog != null && isRunning()) {
       watchdog.destroyProcess();
       watchdog = null;
     }