zeppelin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jongy...@apache.org
Subject zeppelin git commit: [ZEPPELIN-1921] missing close in closeAndRemoveInterpreterGroup method
Date Tue, 24 Jan 2017 06:31:39 GMT
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 <cloverheartsdev@gmail.com>

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 <cloverheartsdev@gmail.com>
Authored: Fri Jan 20 13:38:17 2017 -0800
Committer: Jongyoul Lee <jongyoul@apache.org>
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<String> processList = Arrays.asList(processKey.split(":"));
+      List<String> 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<InterpreterGroup> 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<InterpreterGroup> 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<InterpreterGroup> 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();


Mime
View raw message