Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 18AB3200C04 for ; Tue, 24 Jan 2017 07:31:41 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id 1731A160B4B; Tue, 24 Jan 2017 06:31:41 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 140EB160B3D for ; Tue, 24 Jan 2017 07:31:39 +0100 (CET) Received: (qmail 402 invoked by uid 500); 24 Jan 2017 06:31:39 -0000 Mailing-List: contact commits-help@zeppelin.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zeppelin.apache.org Delivered-To: mailing list commits@zeppelin.apache.org Received: (qmail 393 invoked by uid 99); 24 Jan 2017 06:31:39 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 24 Jan 2017 06:31:39 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 1CAEBDFBAD; Tue, 24 Jan 2017 06:31:39 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: jongyoul@apache.org To: commits@zeppelin.apache.org Message-Id: <03c7e9a2978e4b46a16f78f747e1193c@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: zeppelin git commit: [ZEPPELIN-1921] missing close in closeAndRemoveInterpreterGroup method Date: Tue, 24 Jan 2017 06:31:39 +0000 (UTC) archived-at: Tue, 24 Jan 2017 06:31:41 -0000 Repository: zeppelin Updated Branches: refs/heads/master 92cebe59d -> 3bbcf0969 [ZEPPELIN-1921] missing close in closeAndRemoveInterpreterGroup method ### What is this PR for? The problem is that some code in the closeAndRemoveInterpreterGroup method of InterpreterSetting partially closes. ### What type of PR is it? Bug Fix ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1921 ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: cloverhearts Closes #1864 from cloverhearts/ZEPPELIN-1921 and squashes the following commits: 4ac76cf [cloverhearts] Merge branch 'master' into ZEPPELIN-1921 f39212c [cloverhearts] fixed equals logic e21287f [cloverhearts] apply other side 6b86dfd [cloverhearts] compare logic change 6a2051c [cloverhearts] method name change (master rebase) d12ec57 [cloverhearts] missing brace ca9ecfd [cloverhearts] Merge branch 'master' into ZEPPELIN-1921 22473a2 [cloverhearts] change return logic a105adf [cloverhearts] Merge branch 'master' into ZEPPELIN-1921 b0a9396 [cloverhearts] test case and replace logic 2482be6 [cloverhearts] container method -> isEqualInterpreterKey method e25f311 [cloverhearts] interpreter test case and replace logic 546ee85 [cloverhearts] Revert "change Linkedlist to LinkedHashSet" 59c9c76 [cloverhearts] implement testcase 2188b1b [cloverhearts] change Linkedlist to LinkedHashSet 0ebed44 [cloverhearts] fixed missing for close Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/3bbcf096 Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/3bbcf096 Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/3bbcf096 Branch: refs/heads/master Commit: 3bbcf0969749bfcfc3cb0a58a3af295a44959bd8 Parents: 92cebe5 Author: cloverhearts Authored: Fri Jan 20 13:38:17 2017 -0800 Committer: Jongyoul Lee Committed: Tue Jan 24 15:31:31 2017 +0900 ---------------------------------------------------------------------- .../interpreter/InterpreterSetting.java | 55 +++++++++++++++----- .../interpreter/mock/MockInterpreter11.java | 7 +++ .../notebook/NoteInterpreterLoaderTest.java | 55 ++++++++++++++++++++ 3 files changed, 105 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3bbcf096/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java index 9176ddf..bd7d664 100644 --- a/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java +++ b/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSetting.java @@ -17,6 +17,7 @@ package org.apache.zeppelin.interpreter; +import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -144,6 +145,33 @@ public class InterpreterSetting { return key; } + private boolean isEqualInterpreterKeyProcessKey(String refKey, String processKey) { + InterpreterOption option = getOption(); + int validCount = 0; + if (getOption().isProcess() + && !(option.perUserIsolated() == true && option.perNoteIsolated() == true)) { + + List processList = Arrays.asList(processKey.split(":")); + List refList = Arrays.asList(refKey.split(":")); + + if (refList.size() <= 1 || processList.size() <= 1) { + return refKey.equals(processKey); + } + + if (processList.get(0).equals("") || processList.get(0).equals(refList.get(0))) { + validCount = validCount + 1; + } + + if (processList.get(1).equals("") || processList.get(1).equals(refList.get(1))) { + validCount = validCount + 1; + } + + return (validCount >= 2); + } else { + return refKey.equals(processKey); + } + } + private String getInterpreterSessionKey(String user, String noteId) { InterpreterOption option = getOption(); String key; @@ -194,18 +222,19 @@ public class InterpreterSetting { } void closeAndRemoveInterpreterGroupByNoteId(String noteId) { - String key = getInterpreterProcessKey("", noteId); - - InterpreterGroup groupToRemove = null; + String processKey = getInterpreterProcessKey("", noteId); + List closeToGroupList = new LinkedList<>(); + InterpreterGroup groupKey; for (String intpKey : new HashSet<>(interpreterGroupRef.keySet())) { - if (intpKey.contains(key)) { + if (isEqualInterpreterKeyProcessKey(intpKey, processKey)) { interpreterGroupWriteLock.lock(); - groupToRemove = interpreterGroupRef.remove(intpKey); + groupKey = interpreterGroupRef.remove(intpKey); interpreterGroupWriteLock.unlock(); + closeToGroupList.add(groupKey); } } - if (groupToRemove != null) { + for (InterpreterGroup groupToRemove : closeToGroupList) { groupToRemove.close(); } } @@ -216,17 +245,19 @@ public class InterpreterSetting { } String processKey = getInterpreterProcessKey(user, ""); String sessionKey = getInterpreterSessionKey(user, ""); - InterpreterGroup groupToRemove = null; + List groupToRemove = new LinkedList<>(); + InterpreterGroup groupItem; for (String intpKey : new HashSet<>(interpreterGroupRef.keySet())) { - if (intpKey.contains(processKey)) { + if (isEqualInterpreterKeyProcessKey(intpKey, processKey)) { interpreterGroupWriteLock.lock(); - groupToRemove = interpreterGroupRef.remove(intpKey); + groupItem = interpreterGroupRef.remove(intpKey); interpreterGroupWriteLock.unlock(); + groupToRemove.add(groupItem); } } - if (groupToRemove != null) { - groupToRemove.close(sessionKey); + for (InterpreterGroup groupToClose : groupToRemove) { + groupToClose.close(sessionKey); } } @@ -243,7 +274,7 @@ public class InterpreterSetting { List groupToRemove = new LinkedList<>(); InterpreterGroup groupItem; for (String intpKey : new HashSet<>(interpreterGroupRef.keySet())) { - if (intpKey.contains(key)) { + if (isEqualInterpreterKeyProcessKey(intpKey, key)) { interpreterGroupWriteLock.lock(); groupItem = interpreterGroupRef.remove(intpKey); interpreterGroupWriteLock.unlock(); http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3bbcf096/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter11.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter11.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter11.java index fc30726..58200d8 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter11.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/interpreter/mock/MockInterpreter11.java @@ -35,13 +35,20 @@ public class MockInterpreter11 extends Interpreter{ public MockInterpreter11(Properties property) { super(property); } + boolean open; @Override public void open() { + open = true; } @Override public void close() { + open = false; + } + + public boolean isOpen() { + return open; } @Override http://git-wip-us.apache.org/repos/asf/zeppelin/blob/3bbcf096/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java ---------------------------------------------------------------------- diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java index 22e2039..320a5b5 100644 --- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java +++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteInterpreterLoaderTest.java @@ -20,6 +20,7 @@ import java.io.File; import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.List; import org.apache.zeppelin.conf.ZeppelinConfiguration; import org.apache.zeppelin.conf.ZeppelinConfiguration.ConfVars; @@ -28,13 +29,17 @@ import org.apache.zeppelin.interpreter.Interpreter; import org.apache.zeppelin.interpreter.InterpreterFactory; import org.apache.zeppelin.interpreter.InterpreterOption; import org.apache.zeppelin.interpreter.InterpreterSetting; +import org.apache.zeppelin.interpreter.LazyOpenInterpreter; import org.apache.zeppelin.interpreter.mock.MockInterpreter1; import org.apache.zeppelin.interpreter.mock.MockInterpreter11; import org.apache.zeppelin.interpreter.mock.MockInterpreter2; +import org.apache.zeppelin.interpreter.remote.RemoteInterpreter; +import org.apache.zeppelin.interpreter.remote.RemoteInterpreterProcess; import org.junit.After; import org.junit.Before; import org.junit.Test; +import static java.lang.Thread.sleep; import static org.junit.Assert.*; public class NoteInterpreterLoaderTest { @@ -117,6 +122,11 @@ public class NoteInterpreterLoaderTest { assertNotNull(factory.getInterpreterSettings("noteA").get(0).getInterpreterGroup("user", "noteA").get("noteA")); assertNotNull(factory.getInterpreterSettings("noteB").get(0).getInterpreterGroup("user", "noteB").get("noteB")); + // invalid close + factory.closeNote("user", "note"); + assertNotNull(factory.getInterpreterSettings("noteA").get(0).getInterpreterGroup("user", "shared_process").get("noteA")); + assertNotNull(factory.getInterpreterSettings("noteB").get(0).getInterpreterGroup("user", "shared_process").get("noteB")); + // when factory.closeNote("user", "noteA"); factory.closeNote("user", "noteB"); @@ -160,6 +170,51 @@ public class NoteInterpreterLoaderTest { assertNull(factory.getInterpreterSettings("noteB").get(0).getInterpreterGroup("user", "noteB").get("shared_session")); } + @Test + public void testNoteInterpreterCloseForAll() throws IOException { + factory.setInterpreters("user", "FitstNote", factory.getDefaultInterpreterSettingList()); + factory.getInterpreterSettings("FitstNote").get(0).getOption().setPerNote(InterpreterOption.SCOPED); + + factory.setInterpreters("user", "yourFirstNote", factory.getDefaultInterpreterSettingList()); + factory.getInterpreterSettings("yourFirstNote").get(0).getOption().setPerNote(InterpreterOption.ISOLATED); + + // interpreters are not created before accessing it + assertNull(factory.getInterpreterSettings("FitstNote").get(0).getInterpreterGroup("user", "FitstNote").get("FitstNote")); + assertNull(factory.getInterpreterSettings("yourFirstNote").get(0).getInterpreterGroup("user", "yourFirstNote").get("yourFirstNote")); + + Interpreter firstNoteIntp = factory.getInterpreter("user", "FitstNote", "group1.mock1"); + Interpreter yourFirstNoteIntp = factory.getInterpreter("user", "yourFirstNote", "group1.mock1"); + + firstNoteIntp.open(); + yourFirstNoteIntp.open(); + + assertTrue(((LazyOpenInterpreter)firstNoteIntp).isOpen()); + assertTrue(((LazyOpenInterpreter)yourFirstNoteIntp).isOpen()); + + factory.closeNote("user", "FitstNote"); + + assertFalse(((LazyOpenInterpreter)firstNoteIntp).isOpen()); + assertTrue(((LazyOpenInterpreter)yourFirstNoteIntp).isOpen()); + + //reopen + firstNoteIntp.open(); + + assertTrue(((LazyOpenInterpreter)firstNoteIntp).isOpen()); + assertTrue(((LazyOpenInterpreter)yourFirstNoteIntp).isOpen()); + + // invalid check + factory.closeNote("invalid", "Note"); + + assertTrue(((LazyOpenInterpreter)firstNoteIntp).isOpen()); + assertTrue(((LazyOpenInterpreter)yourFirstNoteIntp).isOpen()); + + // invalid contains value check + factory.closeNote("u", "Note"); + + assertTrue(((LazyOpenInterpreter)firstNoteIntp).isOpen()); + assertTrue(((LazyOpenInterpreter)yourFirstNoteIntp).isOpen()); + } + private void delete(File file){ if(file.isFile()) file.delete();