Repository: zeppelin Updated Branches: refs/heads/master 468cea290 -> 97086be4e
ZEPPELIN-3435. Interpreter timeout lifecycle leads to interpreter process orphans ### What is this PR for? This issue happens when LifecycleManager try to close InterpreterGroup when it is in the middle of starting. This PR change the interface of LifecycleManager, and only add InterpreterGroup to LifecycleManager only when its interpreter process is started. ### What type of PR is it? [Bug Fix] ### Todos * [ ] - Task ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3435 ### How should this be tested? * CI pass ### 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> Closes #2951 from zjffdu/ZEPPELIN-3435 and squashes the following commits: 8b2fde1 [Jeff Zhang] ZEPPELIN-3435. Interpreter timeout lifecycle leads to interpreter process orphans Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/97086be4 Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/97086be4 Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/97086be4 Branch: refs/heads/master Commit: 97086be4ec8e1d9a506303753b886a3a2177578a Parents: 468cea2 Author: Jeff Zhang <zjf...@apache.org> Authored: Sat Apr 28 08:20:45 2018 +0800 Committer: Jeff Zhang <zjf...@apache.org> Committed: Tue May 1 11:27:57 2018 +0800 ---------------------------------------------------------------------- .../org/apache/zeppelin/interpreter/LifecycleManager.java | 5 +---- .../zeppelin/interpreter/ManagedInterpreterGroup.java | 3 +-- .../interpreter/lifecycle/NullLifecycleManager.java | 8 +------- .../interpreter/lifecycle/TimeoutLifecycleManager.java | 10 +++------- .../lifecycle/TimeoutLifecycleManagerTest.java | 9 +++++++-- 5 files changed, 13 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/97086be4/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/LifecycleManager.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/LifecycleManager.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/LifecycleManager.java index fc2a7bd..f36cb0d 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/LifecycleManager.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/LifecycleManager.java @@ -24,10 +24,7 @@ package org.apache.zeppelin.interpreter; */ public interface LifecycleManager { - void onInterpreterGroupCreated(ManagedInterpreterGroup interpreterGroup); - - void onInterpreterSessionCreated(ManagedInterpreterGroup interpreterGroup, - String sessionId); + void onInterpreterProcessStarted(ManagedInterpreterGroup interpreterGroup); void onInterpreterUse(ManagedInterpreterGroup interpreterGroup, String sessionId); http://git-wip-us.apache.org/repos/asf/zeppelin/blob/97086be4/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ManagedInterpreterGroup.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ManagedInterpreterGroup.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ManagedInterpreterGroup.java index e19c9ca..ecbaf16 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ManagedInterpreterGroup.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/ManagedInterpreterGroup.java @@ -48,7 +48,6 @@ public class ManagedInterpreterGroup extends InterpreterGroup { ManagedInterpreterGroup(String id, InterpreterSetting interpreterSetting) { super(id); this.interpreterSetting = interpreterSetting; - interpreterSetting.getLifecycleManager().onInterpreterGroupCreated(this); } public InterpreterSetting getInterpreterSetting() { @@ -63,6 +62,7 @@ public class ManagedInterpreterGroup extends InterpreterGroup { remoteInterpreterProcess = interpreterSetting.createInterpreterProcess(id, userName, properties); remoteInterpreterProcess.start(userName); + interpreterSetting.getLifecycleManager().onInterpreterProcessStarted(this); remoteInterpreterProcess.getRemoteInterpreterEventPoller() .setInterpreterProcess(remoteInterpreterProcess); remoteInterpreterProcess.getRemoteInterpreterEventPoller().setInterpreterGroup(this); @@ -156,7 +156,6 @@ public class ManagedInterpreterGroup extends InterpreterGroup { interpreter.setInterpreterGroup(this); } LOGGER.info("Create Session: {} in InterpreterGroup: {} for user: {}", sessionId, id, user); - interpreterSetting.getLifecycleManager().onInterpreterSessionCreated(this, sessionId); sessions.put(sessionId, interpreters); return interpreters; } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/97086be4/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/NullLifecycleManager.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/NullLifecycleManager.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/NullLifecycleManager.java index ce633c6..5a62d22 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/NullLifecycleManager.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/NullLifecycleManager.java @@ -32,13 +32,7 @@ public class NullLifecycleManager implements LifecycleManager { } @Override - public void onInterpreterGroupCreated(ManagedInterpreterGroup interpreterGroup) { - - } - - @Override - public void onInterpreterSessionCreated(ManagedInterpreterGroup interpreterGroup, - String sessionId) { + public void onInterpreterProcessStarted(ManagedInterpreterGroup interpreterGroup) { } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/97086be4/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java index 7042060..90f3f55 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java @@ -58,18 +58,14 @@ public class TimeoutLifecycleManager implements LifecycleManager { } @Override - public void onInterpreterGroupCreated(ManagedInterpreterGroup interpreterGroup) { + public void onInterpreterProcessStarted(ManagedInterpreterGroup interpreterGroup) { + LOGGER.info("Process of InterpreterGroup {} is started", interpreterGroup.getId()); interpreterGroups.put(interpreterGroup, System.currentTimeMillis()); } @Override - public void onInterpreterSessionCreated(ManagedInterpreterGroup interpreterGroup, - String sessionId) { - - } - - @Override public void onInterpreterUse(ManagedInterpreterGroup interpreterGroup, String sessionId) { + LOGGER.debug("InterpreterGroup {} is used in session {}", interpreterGroup.getId(), sessionId); interpreterGroups.put(interpreterGroup, System.currentTimeMillis()); } } http://git-wip-us.apache.org/repos/asf/zeppelin/blob/97086be4/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManagerTest.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManagerTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManagerTest.java index 329cb7a..1041502 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManagerTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManagerTest.java @@ -54,13 +54,18 @@ public class TimeoutLifecycleManagerTest extends AbstractInterpreterTest { interpreterSettingManager.setInterpreterBinding("user1", "note1", interpreterSettingManager.getSettingIds()); assertTrue(interpreterFactory.getInterpreter("user1", "note1", "test.echo") instanceof RemoteInterpreter); RemoteInterpreter remoteInterpreter = (RemoteInterpreter) interpreterFactory.getInterpreter("user1", "note1", "test.echo"); + assertFalse(remoteInterpreter.isOpened()); + InterpreterSetting interpreterSetting = interpreterSettingManager.getInterpreterSettingByName("test"); + assertEquals(1, interpreterSetting.getAllInterpreterGroups().size()); + Thread.sleep(15*1000); + // InterpreterGroup is not removed after 15 seconds, as TimeoutLifecycleManager only manage it after it is started + assertEquals(1, interpreterSetting.getAllInterpreterGroups().size()); + InterpreterContext context = new InterpreterContext("noteId", "paragraphId", "repl", "title", "text", AuthenticationInfo.ANONYMOUS, new HashMap<String, Object>(), new GUI(), new GUI(), null, null, new ArrayList<InterpreterContextRunner>(), null); remoteInterpreter.interpret("hello world", context); assertTrue(remoteInterpreter.isOpened()); - InterpreterSetting interpreterSetting = interpreterSettingManager.getInterpreterSettingByName("test"); - assertEquals(1, interpreterSetting.getAllInterpreterGroups().size()); Thread.sleep(15 * 1000); // interpreterGroup is timeout, so is removed.
