[ZEPPELIN-1399] Refactor automatic interpreter name insert

### What is this PR for? Refine automatic interpreter name insert. See below jira issue for detailed description ### What type of PR is it? Improvement | Refactoring ### What is the Jira issue? [ZEPPELIN-1399](https://issues.apache.org/jira/browse/ZEPPELIN-1399) ### Screenshots (if appropriate) - Do not insert interpreter name if previous interpreter name is not specified **Before** ![aug-30-2016 17-28-06](https://cloud.githubusercontent.com/assets/8503346/18095394/1f25c9b4-6ed7-11e6-818d-71099a96e305.gif) **After** ![aug-30-2016 17-24-23](https://cloud.githubusercontent.com/assets/8503346/18095264/b469cfc6-6ed6-11e6-8497-320e8f3f8f46.gif) - Insert interpreter name of previous paragraph without running **Before** ![aug-30-2016 17-33-25](https://cloud.githubusercontent.com/assets/8503346/18095643/dfbc1eee-6ed7-11e6-8249-05c4d046da9d.gif) **After** ![aug-30-2016 17-24-32](https://cloud.githubusercontent.com/assets/8503346/18095249/a4936e90-6ed6-11e6-9223-6405f3e36f80.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: Mina Lee <minalee@apache.org> Closes #1388 from minahlee/ZEPPELIN-1399 and squashes the following commits: 6c035d9 [Mina Lee] Modify tests 1484007 [Mina Lee] Refactor automatic magic keyword insert 15cd0d5 [Mina Lee] Revert "[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name"
apache · Sep 7, 2016 · 3db819a · 3db819a
1 parent 66d5811
commit 3db819a
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 160 deletions.
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
@@ -852,7 +852,6 @@ private void handleParagraphParams(String message, Note note, Paragraph paragrap
       if (paramsForUpdating != null) {
         paragraph.settings.getParams().putAll(paramsForUpdating);
         AuthenticationInfo subject = new AuthenticationInfo(SecurityUtils.getPrincipal());
-        note.setLastReplName(paragraph.getId());
         note.persist(subject);
       }
     }

diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
@@ -1130,10 +1130,8 @@ private void runParagraph(NotebookSocket conn, HashSet<String> userAndRoles, Not
        .get("config");
     p.setConfig(config);
     // if it's the last paragraph, let's add a new one
-    boolean isTheLastParagraph = note.getLastParagraph().getId()
-        .equals(p.getId());
-    note.setLastReplName(paragraphId);
-    if (!(text.equals(note.getLastInterpreterName() + " ") || Strings.isNullOrEmpty(text)) &&
+    boolean isTheLastParagraph = note.isLastParagraph(p.getId());
+    if (!(text.trim().equals(p.getMagic()) || Strings.isNullOrEmpty(text)) &&
         isTheLastParagraph) {
       note.addParagraph();
     }

diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/integration/ParagraphActionsIT.java b/zeppelin-server/src/test/java/org/apache/zeppelin/integration/ParagraphActionsIT.java
@@ -95,28 +95,21 @@ public void testCreateNewButton() throws Exception {
       ZeppelinITUtils.sleep(1000, false);
       waitForParagraph(1, "READY");
 
-      String oldIntpTag = driver.findElement(By.xpath(getParagraphXPath(1) + "//div[contains(@class, 'editor')]")).getText();
-
       collector.checkThat("Paragraph is created above",
           driver.findElement(By.xpath(getParagraphXPath(1) + "//div[contains(@class, 'editor')]")).getText(),
-          CoreMatchers.not(StringUtils.EMPTY));
+          CoreMatchers.equalTo(StringUtils.EMPTY));
       setTextOfParagraph(1, " this is above ");
 
-
       newPara = driver.findElement(By.xpath(getParagraphXPath(2) + "//div[contains(@class,'new-paragraph')][2]"));
       action.moveToElement(newPara).click().build().perform();
 
       waitForParagraph(3, "READY");
 
-      String lastIntpTag = driver.findElement(By.xpath(getParagraphXPath(3) + "//div[contains(@class, 'editor')]")).getText();
-
       collector.checkThat("Paragraph is created below",
           driver.findElement(By.xpath(getParagraphXPath(3) + "//div[contains(@class, 'editor')]")).getText(),
-          CoreMatchers.not(StringUtils.EMPTY));
+          CoreMatchers.equalTo(StringUtils.EMPTY));
       setTextOfParagraph(3, " this is below ");
 
-      collector.checkThat("Compare interpreter name tag", oldIntpTag, CoreMatchers.equalTo(lastIntpTag));
-
       collector.checkThat("The output field of paragraph1 contains",
           driver.findElement(By.xpath(getParagraphXPath(1) + "//div[contains(@class, 'editor')]")).getText(),
           CoreMatchers.equalTo(" this is above "));

diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/ZeppelinRestApiTest.java
@@ -146,8 +146,7 @@ public void testNotebookCreateWithParagraphs() throws IOException {
     assertEquals("compare note name", expectedNoteName, newNoteName);
     assertEquals("initial paragraph check failed", 3, newNote.getParagraphs().size());
     for (Paragraph p : newNote.getParagraphs()) {
-      if (StringUtils.isEmpty(p.getText()) ||
-              p.getText().trim().equals(newNote.getLastInterpreterName())) {
+      if (StringUtils.isEmpty(p.getText())) {
         continue;
       }
       assertTrue("paragraph title check failed", p.getTitle().startsWith("title"));

diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java
@@ -29,9 +29,9 @@
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicReference;
 
 import com.google.gson.Gson;
+import org.apache.commons.lang.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -57,11 +57,6 @@
 import org.apache.zeppelin.user.AuthenticationInfo;
 import org.apache.zeppelin.user.Credentials;
 
-import static org.apache.commons.lang.StringUtils.EMPTY;
-import static org.apache.commons.lang.StringUtils.isEmpty;
-import static org.apache.commons.lang.StringUtils.isNotEmpty;
-import static org.apache.commons.lang.StringUtils.isBlank;
-
 /**
  * Binded interpreters for a note
  */
@@ -82,7 +77,6 @@ public class Note implements Serializable, ParagraphJobListener {
   private String name = "";
   private String id;
 
-  private AtomicReference<String> lastReplName = new AtomicReference<>(EMPTY);
   private transient ZeppelinConfiguration conf = ZeppelinConfiguration.create();
 
   private Map<String, List<AngularObject>> angularObjects = new HashMap<>();
@@ -128,13 +122,7 @@ private void generateId() {
 
   private String getDefaultInterpreterName() {
     InterpreterSetting setting = factory.getDefaultInterpreterSetting(getId());
-    return null != setting ? setting.getName() : EMPTY;
-  }
-
-  void putDefaultReplName() {
-    String defaultInterpreterName = getDefaultInterpreterName();
-    logger.info("defaultInterpreterName is '{}'", defaultInterpreterName);
-    lastReplName.set(defaultInterpreterName);
+    return null != setting ? setting.getName() : StringUtils.EMPTY;
   }
 
   public String getId() {
@@ -226,7 +214,7 @@ Map<String, List<AngularObject>> getAngularObjects() {
    */
   public Paragraph addParagraph() {
     Paragraph p = new Paragraph(this, this, factory);
-    addLastReplNameIfEmptyText(p);
+    setParagraphMagic(p, paragraphs.size());
     synchronized (paragraphs) {
       paragraphs.add(p);
     }
@@ -282,7 +270,7 @@ void addCloneParagraph(Paragraph srcParagraph) {
    */
   public Paragraph insertParagraph(int index) {
     Paragraph p = new Paragraph(this, this, factory);
-    addLastReplNameIfEmptyText(p);
+    setParagraphMagic(p, index);
     synchronized (paragraphs) {
       paragraphs.add(index, p);
     }
@@ -292,26 +280,6 @@ public Paragraph insertParagraph(int index) {
     return p;
   }
 
-  /**
-   * Add Last Repl name If Paragraph has empty text
-   *
-   * @param p Paragraph
-   */
-  private void addLastReplNameIfEmptyText(Paragraph p) {
-    String replName = lastReplName.get();
-    if (isEmpty(p.getText()) && isNotEmpty(replName) && isBinding(replName)) {
-      p.setText(getInterpreterName(replName) + " ");
-    }
-  }
-
-  public boolean isBinding(String replName) {
-    return factory.getInterpreter(this.getId(), replName) != null;
-  }
-
-  private String getInterpreterName(String replName) {
-    return isBlank(replName) ? EMPTY : "%" + replName;
-  }
-
   /**
    * Remove paragraph by id.
    *
@@ -439,7 +407,7 @@ public List<Map<String, String>> generateParagraphsInfo() {
     List<Map<String, String>> paragraphsInfo = new LinkedList<>();
     synchronized (paragraphs) {
       for (Paragraph p : paragraphs) {
-        Map<String, String> info = populatePragraphInfo(p);
+        Map<String, String> info = populateParagraphInfo(p);
         paragraphsInfo.add(info);
       }
     }
@@ -450,14 +418,14 @@ public Map<String, String> generateSingleParagraphInfo(String paragraphId) {
     synchronized (paragraphs) {
       for (Paragraph p : paragraphs) {
         if (p.getId().equals(paragraphId)) {
-          return populatePragraphInfo(p);
+          return populateParagraphInfo(p);
         }
       }
       return new HashMap<>();
     }
   }
 
-  private Map<String, String> populatePragraphInfo(Paragraph p) {
+  private Map<String, String> populateParagraphInfo(Paragraph p) {
     Map<String, String> info = new HashMap<>();
     info.put("id", p.getId());
     info.put("status", p.getStatus().toString());
@@ -473,15 +441,26 @@ private Map<String, String> populatePragraphInfo(Paragraph p) {
     return info;
   }
 
+  private void setParagraphMagic(Paragraph p, int index) {
+    if (paragraphs.size() > 0) {
+      String magic;
+      if (index == 0) {
+        magic = paragraphs.get(0).getMagic();
+      } else {
+        magic = paragraphs.get(index - 1).getMagic();
+      }
+      if (StringUtils.isNotEmpty(magic)) {
+        p.setText(magic + "\n");
+      }
+    }
+  }
+
   /**
    * Run all paragraphs sequentially.
    */
   public void runAll() {
     String cronExecutingUser = (String) getConfig().get("cronExecutingUser");
     synchronized (paragraphs) {
-      if (!paragraphs.isEmpty()) {
-        setLastReplName(paragraphs.get(paragraphs.size() - 1));
-      }
       for (Paragraph p : paragraphs) {
         if (!p.isEnabled()) {
           continue;
@@ -607,16 +586,6 @@ public void persist(AuthenticationInfo subject) throws IOException {
     repo.save(this, subject);
   }
 
-  private void setLastReplName(Paragraph lastParagraphStarted) {
-    if (isNotEmpty(lastParagraphStarted.getRequiredReplName())) {
-      lastReplName.set(lastParagraphStarted.getRequiredReplName());
-    }
-  }
-
-  public void setLastReplName(String paragraphId) {
-    setLastReplName(getParagraph(paragraphId));
-  }
-
   /**
    * Persist this note with maximum delay.
    */
@@ -681,14 +650,6 @@ public void setInfo(Map<String, Object> info) {
     this.info = info;
   }
 
-  String getLastReplName() {
-    return lastReplName.get();
-  }
-
-  public String getLastInterpreterName() {
-    return getInterpreterName(getLastReplName());
-  }
-
   @Override
   public void beforeStatusChange(Job job, Status before, Status after) {
     if (jobListenerFactory != null) {
@@ -723,7 +684,6 @@ public void onProgressUpdate(Job job, int progress) {
     }
   }
 
-
   @Override
   public void onOutputAppend(Paragraph paragraph, InterpreterOutput out, String output) {
     if (jobListenerFactory != null) {

diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java
@@ -155,7 +155,6 @@ public Note createNote(List<String> interpreterIds, AuthenticationInfo subject)
     }
     if (interpreterIds != null) {
       bindInterpretersToNote(note.getId(), interpreterIds);
-      note.putDefaultReplName();
     }
 
     notebookIndex.addIndexDoc(note);

diff --git a/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java b/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Paragraph.java
@@ -17,6 +17,7 @@
 
 package org.apache.zeppelin.notebook;
 
+import org.apache.commons.lang.StringUtils;
 import org.apache.zeppelin.display.AngularObject;
 import org.apache.zeppelin.display.AngularObjectRegistry;
 import org.apache.zeppelin.helium.HeliumPackage;
@@ -455,22 +456,22 @@ private InterpreterContext getInterpreterContext(InterpreterOutput output) {
     Credentials credentials = note.getCredentials();
     if (authenticationInfo != null) {
       UserCredentials userCredentials = credentials.getUserCredentials(
-              authenticationInfo.getUser());
+          authenticationInfo.getUser());
       authenticationInfo.setUserCredentials(userCredentials);
     }
 
     InterpreterContext interpreterContext = new InterpreterContext(
-            note.getId(),
-            getId(),
-            this.getTitle(),
-            this.getText(),
-            this.getAuthenticationInfo(),
-            this.getConfig(),
-            this.settings,
-            registry,
-            resourcePool,
-            runners,
-            output);
+        note.getId(),
+        getId(),
+        this.getTitle(),
+        this.getText(),
+        this.getAuthenticationInfo(),
+        this.getConfig(),
+        this.settings,
+        registry,
+        resourcePool,
+        runners,
+        output);
     return interpreterContext;
   }
 
@@ -565,4 +566,22 @@ String extractVariablesFromAngularRegistry(String scriptBody, Map<String, Input>
     }
     return scriptBody;
   }
+
+  public String getMagic() {
+    String magic = StringUtils.EMPTY;
+    String text = getText();
+    if (text != null && text.startsWith("%")) {
+      magic = text.split("\\s+")[0];
+      if (isValidInterpreter(magic.substring(1))) {
+        return magic;
+      } else {
+        return StringUtils.EMPTY;
+      }
+    }
+    return magic;
+  }
+
+  private boolean isValidInterpreter(String replName) {
+    return factory.getInterpreter(note.getId(), replName) != null;
+  }
 }