[GitHub] [zeppelin] asfgit closed pull request #4076: [ZEPPELIN-5284]. savepoint & checkpoint don't work in flink 1.12

[GitBox]

asfgit closed pull request #4076:
URL: https://github.com/apache/zeppelin/pull/4076


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

[GitBox]

asfgit closed pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Teo920127 commented on pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Teo920127 commented on pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#issuecomment-802489730


   In Kubernetes, would it be possible to use customized interpreter image for 
particular interpreters ? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Teo920127 edited a comment on pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Teo920127 edited a comment on pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#issuecomment-802489730


   In Kubernetes, Can interpreters of the same type (such as Python) be 
configured to enable different images?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#issuecomment-802608364


   > In Kubernetes, Can interpreters of the same type (such as Python) be 
configured to enable different images?
   
   At the moment, this is not possible. For a discussion, see 
https://github.com/apache/zeppelin/pull/3769.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#issuecomment-806743948


   The infrastructure tests have been passed. I would like to merge this into 
Master. I think we can also merge this into branch-0.9 as it doesn't break 
anything. What is your opinion @zjffdu and @Leemoonsoo ?
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #3947: [ZEPPELIN-5066] Notebook creation fails in Zeppelin 0.9 with namespace collision

[GitBox]

cuspymd commented on a change in pull request #3947:
URL: https://github.com/apache/zeppelin/pull/3947#discussion_r601994732



##
File path: 
zeppelin-server/src/main/java/org/apache/zeppelin/socket/ConnectionManager.java
##
@@ -359,6 +359,19 @@ public void unicastParagraph(Note note, Paragraph p, 
String user, String msgId)
 }
   }
 
+  public static abstract class UserIterator {
+public abstract void handleUser(String user, Set userAndRoles);
+  }

Review comment:
   This class doesn't iterate a collection. Wouldn't the name `UserHandler` 
or `UserRoleHandler` be more appropriate?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4079: [ZEPPELIN-5288]. Add rest api to reload note

[GitBox]

zjffdu commented on pull request #4079:
URL: https://github.com/apache/zeppelin/pull/4079#issuecomment-807916684


   @cuspymd You are right, thanks for your review 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

zjffdu commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602051890



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   Only jar is transferred ? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

zjffdu commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602051890



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   Only jar file is transferred ? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602089778



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   Yes, that is correct, is there anything else to be transferred?
   Currently, I only have Jar artefacts in the packages section of the 
interpreters, which are downloaded with Maven.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

cuspymd commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602090937



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}
+RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+zeppelinServerHost, port, 3);
+
+RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+intpEventClient, new File(localRepoPath));
+downloader.syncAllLibraries();
+  }
+
+  private void syncAllLibraries() {
+LOGGER.info("Loading all libraries for interpreter {} to {}", interpreter, 
localRepoDir);
+List metadatas = 
client.getAllLibraryMetadatas(interpreter);
+if (!localRepoDir.isDirectory()) {
+  LOGGER.error("{} is no directory", localRepoDir);
+  return;
+}
+Set syncedLibraries = new HashSet<>();
+// Add or update new libraries
+for (LibraryMetadata metadata : metadatas) {
+  File library = new File(localRepoDir, metadata.getName());
+  addOrUpdateLibrary(library, metadata);
+  syncedLibraries.add(metadata.getName());
+}
+// Delete old Jar files
+for (File file : FileUtils.listFiles(localRepoDir, new String[] { "jar" }, 
false)) {
+  if (!syncedLibraries.contains(file.getName())) {
+try {
+  LOGGER.info("Delete {}, because it's not present on the server 
side", file.toPath());
+  Files.delete(file.toPath());
+} catch (IOException e) {
+  LOGGER.error("Unable to delete old library {} during sync.", file, 
e);
+}
+  }
+}
+  }
+
+  private void addOrUpdateLibrary(File library, LibraryMetadata metadata) {
+try {
+  if (library.exists() && library.canRead()) {
+long localChecksum = FileUtils.checksumCRC32(library);
+if (localChecksum == metadata.getChecksum()) {
+  // nothing to do if checksum is matching
+  LOGGER.info("Library {} is present ", library.getName());
+} else {
+  // checksum didn't match
+  Files.delete(library.toPath());
+  downloadLibrary(library, metadata);
+}
+  } else {
+downloadLibrary(library, metadata);
+  }
+} catch (IOException e) {
+  LOGGER.error("Can not add or update library {}", library, e);
+}
+  }
+
+  private void downloadLibrary(File library, LibraryMetadata metadata) {
+LOGGER.debug("Trying to download library {} to {}", metadata.getName(), 
library);
+try {
+  if (library.createNewFile()) {
+int tries = 0;
+while (tries < 3) {
+  ByteBuffer bytes = client.getLibrary(interpreter, 
metadata.getName());
+  FileUtils.writeByteArrayToFile(librar

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602096654



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}
+RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+zeppelinServerHost, port, 3);
+
+RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+intpEventClient, new File(localRepoPath));
+downloader.syncAllLibraries();
+  }
+
+  private void syncAllLibraries() {
+LOGGER.info("Loading all libraries for interpreter {} to {}", interpreter, 
localRepoDir);
+List metadatas = 
client.getAllLibraryMetadatas(interpreter);
+if (!localRepoDir.isDirectory()) {
+  LOGGER.error("{} is no directory", localRepoDir);
+  return;
+}
+Set syncedLibraries = new HashSet<>();
+// Add or update new libraries
+for (LibraryMetadata metadata : metadatas) {
+  File library = new File(localRepoDir, metadata.getName());
+  addOrUpdateLibrary(library, metadata);
+  syncedLibraries.add(metadata.getName());
+}
+// Delete old Jar files
+for (File file : FileUtils.listFiles(localRepoDir, new String[] { "jar" }, 
false)) {
+  if (!syncedLibraries.contains(file.getName())) {
+try {
+  LOGGER.info("Delete {}, because it's not present on the server 
side", file.toPath());
+  Files.delete(file.toPath());
+} catch (IOException e) {
+  LOGGER.error("Unable to delete old library {} during sync.", file, 
e);
+}
+  }
+}
+  }
+
+  private void addOrUpdateLibrary(File library, LibraryMetadata metadata) {
+try {
+  if (library.exists() && library.canRead()) {
+long localChecksum = FileUtils.checksumCRC32(library);
+if (localChecksum == metadata.getChecksum()) {
+  // nothing to do if checksum is matching
+  LOGGER.info("Library {} is present ", library.getName());
+} else {
+  // checksum didn't match
+  Files.delete(library.toPath());
+  downloadLibrary(library, metadata);
+}
+  } else {
+downloadLibrary(library, metadata);
+  }
+} catch (IOException e) {
+  LOGGER.error("Can not add or update library {}", library, e);
+}
+  }
+
+  private void downloadLibrary(File library, LibraryMetadata metadata) {
+LOGGER.debug("Trying to download library {} to {}", metadata.getName(), 
library);
+try {
+  if (library.createNewFile()) {
+int tries = 0;
+while (tries < 3) {
+  ByteBuffer bytes = client.getLibrary(interpreter, 
metadata.getName());
+  FileUtils.writeByteArrayToFile(library

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

zjffdu commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602102340



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   In most of cases it should be OK, Just concern some corner cases that 
some library may depend on non-jar files. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602111388



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   I can add more extensions, but at the moment I don't know which ones.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602129604



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}
+RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+zeppelinServerHost, port, 3);
+
+RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+intpEventClient, new File(localRepoPath));
+downloader.syncAllLibraries();
+  }
+
+  private void syncAllLibraries() {
+LOGGER.info("Loading all libraries for interpreter {} to {}", interpreter, 
localRepoDir);
+List metadatas = 
client.getAllLibraryMetadatas(interpreter);
+if (!localRepoDir.isDirectory()) {
+  LOGGER.error("{} is no directory", localRepoDir);
+  return;
+}
+Set syncedLibraries = new HashSet<>();
+// Add or update new libraries
+for (LibraryMetadata metadata : metadatas) {
+  File library = new File(localRepoDir, metadata.getName());
+  addOrUpdateLibrary(library, metadata);
+  syncedLibraries.add(metadata.getName());
+}
+// Delete old Jar files
+for (File file : FileUtils.listFiles(localRepoDir, new String[] { "jar" }, 
false)) {
+  if (!syncedLibraries.contains(file.getName())) {
+try {
+  LOGGER.info("Delete {}, because it's not present on the server 
side", file.toPath());
+  Files.delete(file.toPath());
+} catch (IOException e) {
+  LOGGER.error("Unable to delete old library {} during sync.", file, 
e);
+}
+  }
+}
+  }
+
+  private void addOrUpdateLibrary(File library, LibraryMetadata metadata) {
+try {
+  if (library.exists() && library.canRead()) {
+long localChecksum = FileUtils.checksumCRC32(library);
+if (localChecksum == metadata.getChecksum()) {
+  // nothing to do if checksum is matching
+  LOGGER.info("Library {} is present ", library.getName());
+} else {
+  // checksum didn't match
+  Files.delete(library.toPath());
+  downloadLibrary(library, metadata);
+}
+  } else {
+downloadLibrary(library, metadata);
+  }
+} catch (IOException e) {
+  LOGGER.error("Can not add or update library {}", library, e);
+}
+  }
+
+  private void downloadLibrary(File library, LibraryMetadata metadata) {
+LOGGER.debug("Trying to download library {} to {}", metadata.getName(), 
library);
+try {
+  if (library.createNewFile()) {
+int tries = 0;
+while (tries < 3) {
+  ByteBuffer bytes = client.getLibrary(interpreter, 
metadata.getName());
+  FileUtils.writeByteArrayToFile(library

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

zjffdu commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r602144305



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/RemoteInterpreterEventServer.java
##
@@ -557,4 +563,58 @@ public void updateParagraphConfig(String noteId,
   LOGGER.error("Fail to updateParagraphConfig", e);
 }
   }
+
+  @Override
+  public List getAllLibraryMetadatas(String interpreter) 
throws TException {
+if (StringUtils.isBlank(interpreter)) {
+  LOGGER.warn("Interpreter is blank");
+  return Collections.emptyList();
+}
+File interpreterLocalRepo = new File(
+
zConf.getAbsoluteDir(ZeppelinConfiguration.ConfVars.ZEPPELIN_DEP_LOCALREPO)
++ File.separator
++ interpreter);
+if (!interpreterLocalRepo.exists()) {
+  LOGGER.warn("Local interpreter repository {} for interpreter {} doesn't 
exists", interpreterLocalRepo,
+  interpreter);
+  return Collections.emptyList();
+}
+if (!interpreterLocalRepo.isDirectory()) {
+  LOGGER.warn("Local interpreter repository {} is no folder", 
interpreterLocalRepo);
+  return Collections.emptyList();
+}
+Collection files = FileUtils.listFiles(interpreterLocalRepo, new 
String[] { "jar" }, false);

Review comment:
   Yeah, I think it should be OK for now.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4080: [ZEPPELIN-5300] Paragraph pending until timeout when process launcher has already fails

[GitBox]

zjffdu opened a new pull request #4080:
URL: https://github.com/apache/zeppelin/pull/4080


   
   ### What is this PR for?
   
   This PR is to exist the waitForReady method earlier when process launcher is 
failed. Currently it would only exit when launch timeout.
   
   ### What type of PR is it?
   [Improvement ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5300
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4081: [ZEPPELIN-5299]. Comment at end of query causes query to be ignored

[GitBox]

zjffdu opened a new pull request #4081:
URL: https://github.com/apache/zeppelin/pull/4081


   ### What is this PR for?
   
   This is to fix the corner case that when the comment is at the end of query, 
the query will be skipped due to bug in SqlSplitter.
   This PR fix the bug in SqlSplitter and also add UT
   
   ### What type of PR is it?
   [Bug Fix ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5299
   
   ### How should this be tested?
   * UT is added
   
   ### Screenshots (if appropriate)
   
   ### Questions: 
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4080: [ZEPPELIN-5300] Paragraph pending until timeout when process launcher has already fails

[GitBox]

cuspymd commented on a change in pull request #4080:
URL: https://github.com/apache/zeppelin/pull/4080#discussion_r602709246



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/ExecRemoteInterpreterProcess.java
##
@@ -175,6 +177,7 @@ public void waitForShutdown(int timeout) {
 
 @Override
 public void waitForReady(int timeout) {
+  this.waitForThread = Thread.currentThread();
   synchronized (this) {

Review comment:
   Wouldn't it be better to be inside the synchronized block?
   And is it okay to not assign `this.waitForThread` to null again?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4077: [ZEPPELIN-5273] Support Spark 3.1.1

[GitBox]

zjffdu commented on pull request #4077:
URL: https://github.com/apache/zeppelin/pull/4077#issuecomment-808723651


   LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4077: [ZEPPELIN-5273] Support Spark 3.1.1

[GitBox]

Reamer commented on pull request #4077:
URL: https://github.com/apache/zeppelin/pull/4077#issuecomment-809162672


   LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Leemoonsoo commented on pull request #4077: [ZEPPELIN-5273] Support Spark 3.1.1

[GitBox]

Leemoonsoo commented on pull request #4077:
URL: https://github.com/apache/zeppelin/pull/4077#issuecomment-809400582


   Thank @Reamer and @zjffdu for the review and guide this PR.
   I'm merging this one to master.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4077: [ZEPPELIN-5273] Support Spark 3.1.1

[GitBox]

asfgit closed pull request #4077:
URL: https://github.com/apache/zeppelin/pull/4077


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] eljur commented on pull request #3712: [ZEPPELIN-4721]Fix the ConcurrentModificationException occured when connect presto via JDBC generic interpreter

[GitBox]

eljur commented on pull request #3712:
URL: https://github.com/apache/zeppelin/pull/3712#issuecomment-809625313


   I just tried preview2, it throws NoClassDefFoundError for hive interpreter 
and does not fix presto one. Is it expected?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #3712: [ZEPPELIN-4721]Fix the ConcurrentModificationException occured when connect presto via JDBC generic interpreter

[GitBox]

zjffdu commented on pull request #3712:
URL: https://github.com/apache/zeppelin/pull/3712#issuecomment-809853017


   @eljur Your issue should not related with this PR, you can try 0.9.0


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

cuspymd commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603758497



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private static final int MAX_LIBRARY_DOWNLOAD_ATTEMPTS = 3;
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}

Review comment:
   It seems that the argument check is not strict because the location 
where this file is used is fixed. All four arguments seem be mandatory, 
Wouldn't it be better to add an `assert` to work only with "`args.length` == 4"?

##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private static final int MAX_LIBRARY_DOWNLOAD_ATTEMPTS = 3;
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String loca

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603865473



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private static final int MAX_LIBRARY_DOWNLOAD_ATTEMPTS = 3;
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}
+RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+zeppelinServerHost, port, 3);
+
+RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+intpEventClient, new File(localRepoPath));
+downloader.syncAllLibraries();
+  }
+
+  private void syncAllLibraries() {
+LOGGER.info("Loading all libraries for interpreter {} to {}", interpreter, 
localRepoDir);
+List metadatas = 
client.getAllLibraryMetadatas(interpreter);
+if (!localRepoDir.isDirectory()) {
+  LOGGER.error("{} is no directory", localRepoDir);
+  return;
+}
+Set syncedLibraries = new HashSet<>();
+// Add or update new libraries
+for (LibraryMetadata metadata : metadatas) {
+  File library = new File(localRepoDir, metadata.getName());
+  addOrUpdateLibrary(library, metadata);
+  syncedLibraries.add(metadata.getName());
+}
+// Delete old Jar files
+for (File file : FileUtils.listFiles(localRepoDir, new String[] { "jar" }, 
false)) {
+  if (!syncedLibraries.contains(file.getName())) {
+try {
+  LOGGER.info("Delete {}, because it's not present on the server 
side", file.toPath());
+  Files.delete(file.toPath());
+} catch (IOException e) {
+  LOGGER.error("Unable to delete old library {} during sync.", file, 
e);
+}
+  }
+}
+  }
+
+  private void addOrUpdateLibrary(File library, LibraryMetadata metadata) {
+try {
+  if (library.exists() && library.canRead()) {
+long localChecksum = FileUtils.checksumCRC32(library);
+if (localChecksum == metadata.getChecksum()) {
+  // nothing to do if checksum is matching
+  LOGGER.info("Library {} is present ", library.getName());
+} else {
+  // checksum didn't match
+  Files.delete(library.toPath());
+  downloadLibrary(library, metadata);
+}
+  } else {
+downloadLibrary(library, metadata);
+  }
+} catch (IOException e) {
+  LOGGER.error("Can not add or update library {}", library, e);
+}
+  }
+
+  private void downloadLibrary(File library, LibraryMetadata metadata) {
+LOGGER.debug("Trying to download library {} to {}", metadata.getName(), 
library);
+try {
+  if (library.createNewFile()) {
+int attempt = 0;
+while (attempt < MAX_LIBRARY_DOWNLOAD_ATTEMPTS) {
+  ByteBuffer bytes = clien

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603876767



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private static final int MAX_LIBRARY_DOWNLOAD_ATTEMPTS = 3;
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}

Review comment:
   You are right. I have changed the implementation to properly evaluate 
the mandatory arguments.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

cuspymd commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603921780



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.interpreter.remote;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.file.Files;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+
+import org.apache.commons.io.FileUtils;
+import org.apache.zeppelin.interpreter.Constants;
+import org.apache.zeppelin.interpreter.thrift.LibraryMetadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class RemoteInterpreterDownloader {
+  private static final Logger LOGGER = 
LoggerFactory.getLogger(RemoteInterpreterDownloader.class);
+
+  private static final int MAX_LIBRARY_DOWNLOAD_ATTEMPTS = 3;
+
+  private final RemoteInterpreterEventClient client;
+  private final String interpreter;
+  private final File localRepoDir;
+
+  public RemoteInterpreterDownloader(
+  String interpreter,
+  RemoteInterpreterEventClient client,
+  File localRepoDir) {
+this.client = client;
+this.interpreter = interpreter;
+this.localRepoDir = localRepoDir;
+  }
+
+  public static void main(String[] args) {
+String zeppelinServerHost = null;
+int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
+String interpreter = null;
+String localRepoPath = null;
+if (args.length > 0) {
+  zeppelinServerHost = args[0];
+  port = Integer.parseInt(args[1]);
+  interpreter = args[2];
+  localRepoPath = args[3];
+}
+RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+zeppelinServerHost, port, 3);
+
+RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+intpEventClient, new File(localRepoPath));
+downloader.syncAllLibraries();
+  }
+
+  private void syncAllLibraries() {
+LOGGER.info("Loading all libraries for interpreter {} to {}", interpreter, 
localRepoDir);
+List metadatas = 
client.getAllLibraryMetadatas(interpreter);
+if (!localRepoDir.isDirectory()) {
+  LOGGER.error("{} is no directory", localRepoDir);
+  return;
+}
+Set syncedLibraries = new HashSet<>();
+// Add or update new libraries
+for (LibraryMetadata metadata : metadatas) {
+  File library = new File(localRepoDir, metadata.getName());
+  addOrUpdateLibrary(library, metadata);
+  syncedLibraries.add(metadata.getName());
+}
+// Delete old Jar files
+for (File file : FileUtils.listFiles(localRepoDir, new String[] { "jar" }, 
false)) {
+  if (!syncedLibraries.contains(file.getName())) {
+try {
+  LOGGER.info("Delete {}, because it's not present on the server 
side", file.toPath());
+  Files.delete(file.toPath());
+} catch (IOException e) {
+  LOGGER.error("Unable to delete old library {} during sync.", file, 
e);
+}
+  }
+}
+  }
+
+  private void addOrUpdateLibrary(File library, LibraryMetadata metadata) {
+try {
+  if (library.exists() && library.canRead()) {
+long localChecksum = FileUtils.checksumCRC32(library);
+if (localChecksum == metadata.getChecksum()) {
+  // nothing to do if checksum is matching
+  LOGGER.info("Library {} is present ", library.getName());
+} else {
+  // checksum didn't match
+  Files.delete(library.toPath());
+  downloadLibrary(library, metadata);
+}
+  } else {
+downloadLibrary(library, metadata);
+  }
+} catch (IOException e) {
+  LOGGER.error("Can not add or update library {}", library, e);
+}
+  }
+
+  private void downloadLibrary(File library, LibraryMetadata metadata) {
+LOGGER.debug("Trying to download library {} to {}", metadata.getName(), 
library);
+try {
+  if (library.createNewFile()) {
+int attempt = 0;
+while (attempt < MAX_LIBRARY_DOWNLOAD_ATTEMPTS) {
+  ByteBuffer bytes = clie

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

cuspymd commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603933999



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -51,22 +50,21 @@ public RemoteInterpreterDownloader(
   }
 
   public static void main(String[] args) {
-String zeppelinServerHost = null;
-int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
-String interpreter = null;
-String localRepoPath = null;
-if (args.length > 0) {
-  zeppelinServerHost = args[0];
-  port = Integer.parseInt(args[1]);
-  interpreter = args[2];
-  localRepoPath = args[3];
-}
-RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
-zeppelinServerHost, port, 3);
+if (args.length == 4) {
+  String zeppelinServerHost = args[0];
+  int port = Integer.parseInt(args[1]);
+  String interpreter = args[2];
+  String localRepoPath = args[3];
+  RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+  zeppelinServerHost, port, 3);
 
-RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
-intpEventClient, new File(localRepoPath));
-downloader.syncAllLibraries();
+  RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+  intpEventClient, new File(localRepoPath));
+  downloader.syncAllLibraries();
+} else {
+  LOGGER.error(
+  "Wrong amount of Arguments. Expected: [ZeppelinHostname] 
[ZeppelinPort] [InterpreterName] [LocalRepoPath]");
+}

Review comment:
   In this case, the script executing this program cannot determine that 
this program has not been executed normally due to the mismatch of the 
arguments. Is this intentional?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on a change in pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#discussion_r603963761



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterDownloader.java
##
@@ -51,22 +50,21 @@ public RemoteInterpreterDownloader(
   }
 
   public static void main(String[] args) {
-String zeppelinServerHost = null;
-int port = Constants.ZEPPELIN_INTERPRETER_DEFAUlT_PORT;
-String interpreter = null;
-String localRepoPath = null;
-if (args.length > 0) {
-  zeppelinServerHost = args[0];
-  port = Integer.parseInt(args[1]);
-  interpreter = args[2];
-  localRepoPath = args[3];
-}
-RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
-zeppelinServerHost, port, 3);
+if (args.length == 4) {
+  String zeppelinServerHost = args[0];
+  int port = Integer.parseInt(args[1]);
+  String interpreter = args[2];
+  String localRepoPath = args[3];
+  RemoteInterpreterEventClient intpEventClient = new 
RemoteInterpreterEventClient(
+  zeppelinServerHost, port, 3);
 
-RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
-intpEventClient, new File(localRepoPath));
-downloader.syncAllLibraries();
+  RemoteInterpreterDownloader downloader = new 
RemoteInterpreterDownloader(interpreter,
+  intpEventClient, new File(localRepoPath));
+  downloader.syncAllLibraries();
+} else {
+  LOGGER.error(
+  "Wrong amount of Arguments. Expected: [ZeppelinHostname] 
[ZeppelinPort] [InterpreterName] [LocalRepoPath]");
+}

Review comment:
   Thanks for your review, an exit code has been added.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer opened a new pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plug-ins in our CI infrastructure

[GitBox]

Reamer opened a new pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082


   ### What is this PR for?
   Activates the tests for all Zeppelin plug-ins in our CI infrastructure
   
   ### What type of PR is it?
- Bug Fix
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5305
   
   ### How should this be tested?
   * CI
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

cuspymd commented on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-810734456


   I have one question. Some tests sometimes cause errors, why is this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer edited a comment on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

Reamer edited a comment on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-810852869


   >  have one question. Some tests sometimes cause errors, why is this?
   
   At the moment we have a CI problem. I have informed the other devs via our 
[mailing 
list](https://lists.apache.org/thread.html/r7555a8a39a493ef3588d7000e450fd7801affac14b970b8f8c24b33b%40%3Cdev.zeppelin.apache.org%3E).
   
   I see several reasons for the rest of the time.
- some parts of our application and test code are quite old and could be 
optimised
- Zeppelin has a large number of external dependencies, which are also 
needed in the integration tests. It starts with the programming language, e.g. 
Python, R, and ends with other large projects such as Flink, Spark, Hadoop, etc.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

Reamer commented on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-810852869


   At the moment we have a CI problem. I have informed the other devs via our 
[mailing 
list](https://lists.apache.org/thread.html/r7555a8a39a493ef3588d7000e450fd7801affac14b970b8f8c24b33b%40%3Cdev.zeppelin.apache.org%3E).
   
   I see several reasons for the rest of the time.
- some parts of our application and test code are quite old and could be 
optimised
- Zeppelin has a large number of external dependencies, which are also 
needed in the integration tests. It starts with the programming language, e.g. 
Python, R, and ends with other large projects such as Flink, Spark, Hadoop, etc.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

cuspymd commented on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-810933409


   
   > At the moment we have a CI problem. I have informed the other devs via our 
[mailing 
list](https://lists.apache.org/thread.html/r7555a8a39a493ef3588d7000e450fd7801affac14b970b8f8c24b33b%40%3Cdev.zeppelin.apache.org%3E).
   > 
   > I see several reasons for the rest of the time.
   > 
   > * some parts of our application and test code are quite old and could 
be optimised
   > 
   > * Zeppelin has a large number of external dependencies, which are also 
needed in the integration tests. It starts with the programming language, e.g. 
Python, R, and ends with other large projects such as Flink, Spark, Hadoop, etc.
   
   Thanks for your detailed explanation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4083: [ZEPPELIN-5307] Ignore the single quote and double quote in sql comment

[GitBox]

zjffdu opened a new pull request #4083:
URL: https://github.com/apache/zeppelin/pull/4083


   ### What is this PR for?
   
   This PR is to fix the bug of SqlSplitter when single/double quote in sql 
comment. UT is added. 
   
   ### What type of PR is it?
   [Bug Fix]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5307
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4084: [ZEPPELIN-5296]. NPE when calling completion for %spark.sql

[GitBox]

zjffdu opened a new pull request #4084:
URL: https://github.com/apache/zeppelin/pull/4084


   ### What is this PR for?
   
   Trivial PR to fix NPE in Spark interpreter when calling code completion. 
   
   ### What type of PR is it?
   [Bug Fix 
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5296
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4081: [ZEPPELIN-5299]. Comment at end of query causes query to be ignored

[GitBox]

zjffdu commented on pull request #4081:
URL: https://github.com/apache/zeppelin/pull/4081#issuecomment-812278438


   Will merge if no more comment


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4085: [ZEPPELIN-5255] Exported note name should included the note id

[GitBox]

zjffdu opened a new pull request #4085:
URL: https://github.com/apache/zeppelin/pull/4085


   ### What is this PR for?
   
   Trivial PR to include note id into the exported note name so that the 
exported note name is consistent with the note name we stored in notebook repo.
   
   ### What type of PR is it?
   [Improvement]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5255
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4086: [ZEPPELIN-5243]. Get HIVE_CONF_DIR from enviroment in flink interpreter

[GitBox]

zjffdu opened a new pull request #4086:
URL: https://github.com/apache/zeppelin/pull/4086


   ### What is this PR for?
   
   Trivial PR to get HIVE_CONF_DIR from environment variable in flink 
interpreter
   
   ### What type of PR is it?
   [Bug Fix]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5243
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4087: [ZEPPELIN-5234]. Increase default value of ZEPPELIN_INTERPRETER_CONNECTION_POOL_SIZE

[GitBox]

zjffdu opened a new pull request #4087:
URL: https://github.com/apache/zeppelin/pull/4087


   ### What is this PR for?
   
   Trivial PR to increase the default value of 
`ZEPPELIN_INTERPRETER_CONNECTION_POOL_SIZE`
   
   ### What type of PR is it?
   [ Improvement ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5234
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4083: [ZEPPELIN-5307] Ignore the single quote and double quote in sql comment

[GitBox]

cuspymd commented on a change in pull request #4083:
URL: https://github.com/apache/zeppelin/pull/4083#discussion_r606124319



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/SqlSplitter.java
##
@@ -102,15 +102,15 @@ public SqlSplitter(String... 
additionalSingleCommentPrefixList) {
 multiLineComment = false;
   }
 
-  if (character == '\'') {
+  if (character == '\'' && (!singleLineComment && !multiLineComment)) {

Review comment:
   As shown below, moving "!" out of parentheses will make it easier to 
understand the conditions.
   ```
   if (character == '\'' && !(singleLineComment || multiLineComment)) {
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

zjffdu commented on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-812404735


   LGTM, thanks @Reamer 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4083: [ZEPPELIN-5307] Ignore the single quote and double quote in sql comment

[GitBox]

zjffdu commented on a change in pull request #4083:
URL: https://github.com/apache/zeppelin/pull/4083#discussion_r606133784



##
File path: 
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/util/SqlSplitter.java
##
@@ -102,15 +102,15 @@ public SqlSplitter(String... 
additionalSingleCommentPrefixList) {
 multiLineComment = false;
   }
 
-  if (character == '\'') {
+  if (character == '\'' && (!singleLineComment && !multiLineComment)) {

Review comment:
   Addressed




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

Reamer commented on pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082#issuecomment-812438401


   I will merge this into Master and Branch-0.9 next week.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4079: [ZEPPELIN-5288]. Add rest api to reload note

[GitBox]

cuspymd commented on a change in pull request #4079:
URL: https://github.com/apache/zeppelin/pull/4079#discussion_r606168837



##
File path: 
zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
##
@@ -78,6 +78,39 @@ public void setUp() {
 anonymous = new AuthenticationInfo("anonymous");
   }
 
+  @Test
+  public void testGetReloadNote() throws IOException {
+LOG.info("Running testGetNote");
+Note note1 = null;
+try {
+  note1 = TestUtils.getInstance(Notebook.class).createNote("note1", 
anonymous);
+  note1.addNewParagraph(AuthenticationInfo.ANONYMOUS);
+  TestUtils.getInstance(Notebook.class).saveNote(note1, anonymous);
+  CloseableHttpResponse get = httpGet("/notebook/" + note1.getId());
+  assertThat(get, isAllowed());
+  Map resp = 
gson.fromJson(EntityUtils.toString(get.getEntity(), StandardCharsets.UTF_8),
+  new TypeToken>() {}.getType());

Review comment:
   It is repeated in all tests, and it would be good to separate it into a 
function later.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4086: [ZEPPELIN-5243]. Get HIVE_CONF_DIR from enviroment in flink interpreter

[GitBox]

cuspymd commented on a change in pull request #4086:
URL: https://github.com/apache/zeppelin/pull/4086#discussion_r606174794



##
File path: 
flink/interpreter/src/main/scala/org/apache/zeppelin/flink/FlinkScalaInterpreter.scala
##
@@ -450,9 +450,9 @@ class FlinkScalaInterpreter(val properties: Properties) {
   }
 
   private def registerHiveCatalog(): Unit = {
-val hiveConfDir =
-  properties.getOrDefault("HIVE_CONF_DIR", 
System.getenv("HIVE_CONF_DIR")).toString
-if (hiveConfDir == null) {
+val hiveConfDir = sys.env.getOrElse("HIVE_CONF_DIR",
+  properties.getProperty("HIVE_CONF_DIR", "")).toString

Review comment:
   The priority seems be changed between environment variable and property, 
is it intentional?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4083: [ZEPPELIN-5307] Ignore the single quote and double quote in sql comment

[GitBox]

zjffdu commented on pull request #4083:
URL: https://github.com/apache/zeppelin/pull/4083#issuecomment-813027604


   Thanks for the review @cuspymd Will merge if no more comment


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4079: [ZEPPELIN-5288]. Add rest api to reload note

[GitBox]

zjffdu commented on a change in pull request #4079:
URL: https://github.com/apache/zeppelin/pull/4079#discussion_r606797662



##
File path: 
zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
##
@@ -78,6 +78,39 @@ public void setUp() {
 anonymous = new AuthenticationInfo("anonymous");
   }
 
+  @Test
+  public void testGetReloadNote() throws IOException {
+LOG.info("Running testGetNote");
+Note note1 = null;
+try {
+  note1 = TestUtils.getInstance(Notebook.class).createNote("note1", 
anonymous);
+  note1.addNewParagraph(AuthenticationInfo.ANONYMOUS);
+  TestUtils.getInstance(Notebook.class).saveNote(note1, anonymous);
+  CloseableHttpResponse get = httpGet("/notebook/" + note1.getId());
+  assertThat(get, isAllowed());
+  Map resp = 
gson.fromJson(EntityUtils.toString(get.getEntity(), StandardCharsets.UTF_8),
+  new TypeToken>() {}.getType());

Review comment:
   That's right, there're many places in test that needs to do code 
refactoring, this could be done in a separated PR. If you would like to make 
contribution in Zeppelin, you can start with that I believe. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4088: SPARK_HOME in zeppelin-env.sh doesn't take effect in yarn cluster mode

[GitBox]

zjffdu opened a new pull request #4088:
URL: https://github.com/apache/zeppelin/pull/4088


   ### What is this PR for?
   
   The root cause is that we didn't get SPARK_HOME from env first when 
detecting scala version. 
   
   ### What type of PR is it?
   [Bug Fix ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5310
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4085: [ZEPPELIN-5255] Exported note name should included the note id

[GitBox]

zjffdu commented on pull request #4085:
URL: https://github.com/apache/zeppelin/pull/4085#issuecomment-813769369


   Will merge if no more comment


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4084: [ZEPPELIN-5296]. NPE when calling completion for %spark.sql

[GitBox]

zjffdu commented on pull request #4084:
URL: https://github.com/apache/zeppelin/pull/4084#issuecomment-813769536


   Will merge if no more comment


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4083: [ZEPPELIN-5307] Ignore the single quote and double quote in sql comment

[GitBox]

asfgit closed pull request #4083:
URL: https://github.com/apache/zeppelin/pull/4083


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4082: [ZEPPELIN-5305] Activate tests for Zeppelin plugins in our CI infrastructure

[GitBox]

asfgit closed pull request #4082:
URL: https://github.com/apache/zeppelin/pull/4082


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4088: SPARK_HOME in zeppelin-env.sh doesn't take effect in yarn cluster mode

[GitBox]

zjffdu commented on pull request #4088:
URL: https://github.com/apache/zeppelin/pull/4088#issuecomment-814566829


   @cuspymd Could you point where it inconsistent ? Any place we take 
environment variable first ? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4087: [ZEPPELIN-5234]. Increase default value of ZEPPELIN_INTERPRETER_CONNECTION_POOL_SIZE

[GitBox]

asfgit closed pull request #4087:
URL: https://github.com/apache/zeppelin/pull/4087


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4084: [ZEPPELIN-5296]. NPE when calling completion for %spark.sql

[GitBox]

asfgit closed pull request #4084:
URL: https://github.com/apache/zeppelin/pull/4084


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4085: [ZEPPELIN-5255] Exported note name should included the note id

[GitBox]

asfgit closed pull request #4085:
URL: https://github.com/apache/zeppelin/pull/4085


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on pull request #4088: [ZEPPELIN-5310] SPARK_HOME in zeppelin-env.sh doesn't take effect in yarn cluster mode

[GitBox]

cuspymd commented on pull request #4088:
URL: https://github.com/apache/zeppelin/pull/4088#issuecomment-814575595


   > 
   > 
   > @cuspymd Could you point where it inconsistent ? Any place we take 
environment variable first ?
   
   Oh, I'm not saying because there is an error in the code. You don't need to 
care about it, because I don't have a good understanding of the code yet, so it 
may feel that way.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4088: [ZEPPELIN-5310] SPARK_HOME in zeppelin-env.sh doesn't take effect in yarn cluster mode

[GitBox]

zjffdu commented on pull request #4088:
URL: https://github.com/apache/zeppelin/pull/4088#issuecomment-814578428


   No problem  @cuspymd Thanks for your time on the code review. Very 
appreciated !


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4081: [ZEPPELIN-5299] Comment at end of query causes query to be ignored

[GitBox]

asfgit closed pull request #4081:
URL: https://github.com/apache/zeppelin/pull/4081


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] PrarthiJain opened a new pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

PrarthiJain opened a new pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089


   ### What is this PR for?
   • This PR is to upgrade thrift to 0.14.1
   
   ### What type of PR is it?
   • [Improvement]
   
   ### Todos
   • [ ] - Task
   
   ### What is the Jira issue?
   • https://issues.apache.org/jira/browse/ZEPPELIN-5314
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] PrarthiJain commented on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

PrarthiJain commented on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-815744305


   @prabhjyotsingh @VipinRathor @zjffdu, Could you please help in reviewing? 
Thanks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

Reamer commented on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-815769361


   Please remove your changes in the zeppelin-web submodule. I closed your JIRA 
ticket because it duplicates another one. Please change your PR title and link 
to your PR text.
   
   Maybe we can remove the `System.exit(0)` for a clean shutdown of the 
Zeppelin interpreter. It would be nice if you could test this. See 
https://issues.apache.org/jira/browse/ZEPPELIN-5249


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd opened a new pull request #4090: [ZEPPELIN-5316] Incorrect markdown at '/quickstart/docker.html'

[GitBox]

cuspymd opened a new pull request #4090:
URL: https://github.com/apache/zeppelin/pull/4090


   ### What is this PR for?
   Fix incorrect markdown at /quickstart/docker.md
   
   ### What type of PR is it?
   [Documentation]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5316
   
   ### How should this be tested?
   * Checked update document locally
   
   ### Screenshots (if appropriate)
   
![image](https://user-images.githubusercontent.com/8870299/114047424-a38ef880-98c4-11eb-8a74-26e03336d22f.png)
   
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

Reamer commented on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-816508269


   This PR also affects #4072, which still uses the old Thrift version.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] PrarthiJain commented on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

PrarthiJain commented on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-816593386


   Thanks, @Reamer. Could you please help with the steps to test for a 
successful shutdown of the Zeppelin interpreter?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer edited a comment on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

Reamer edited a comment on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-815769361


   Please remove your changes in the zeppelin-web submodule. I closed your JIRA 
ticket because it duplicates another one. Please change your PR title and link 
in your PR text.
   
   Maybe we can remove the `System.exit(0)` for a clean shutdown of the 
Zeppelin interpreter. It would be nice if you could test this. See 
https://issues.apache.org/jira/browse/ZEPPELIN-5249


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4089: [ZEPPELIN-5314]. Upgrade thrift to 0.14.1

[GitBox]

Reamer commented on pull request #4089:
URL: https://github.com/apache/zeppelin/pull/4089#issuecomment-816623685


   > Thanks, @Reamer. Could you please help with the steps to test for a 
successful shutdown of the Zeppelin interpreter?
   
   After #4072 has been merged, I can help you,


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd opened a new pull request #4091: [ZEPPELIN-5319] Incorrect markdown in the documentation of sap interpreter

[GitBox]

cuspymd opened a new pull request #4091:
URL: https://github.com/apache/zeppelin/pull/4091


   ### What is this PR for?
   Fix incorrect example code display at "/interpreter/sap.html"
   
   
   ### What type of PR is it?
   [Documentation]
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5319
   
   
   ### How should this be tested?
   * Checked modified document locally
   
   ### Screenshots (if appropriate)
   
![image](https://user-images.githubusercontent.com/8870299/114196484-5fffc180-998c-11eb-9fb3-d23e1a01625c.png)
   
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer opened a new pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer opened a new pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092


   ### What is this PR for?
   When a user renames a note Zeppelin does not check for existing notes with 
the same name/path. This can result in errors as multiple notes can have 
conflicting paths.
   
   ### What type of PR is it?
   Bug Fix
   
   ### Todos
   * [x] - Add validation to Java code
   * [x] - Add java unit tests
   * [x] - Update Note UI to revert name to previous name when update fails
   * [x] - Add unit tests for UI
   
   ### What is the Jira issue?
   * [ZEPPELIN-4506](https://issues.apache.org/jira/browse/ZEPPELIN-4506)
   
   ### How should this be tested?
   * Java unit tests
   * JavaScript unit tests
   * Manual unit test
   
   ### Screenshots (if appropriate)
   
![test__-_Zeppelin](https://user-images.githubusercontent.com/4950503/114375043-30171080-9b7c-11eb-81fe-0869a5a395dc.png)
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

Reamer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611652026



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##
@@ -225,6 +228,8 @@ public void moveNote(String noteId,
   throw new IOException("No metadata found for this note: " + noteId);
 }
 
+validateNotePathDoesNotExist(newNotePath);

Review comment:
   Just a style change:
   Please return a boolean value and do not throw the IOExecption out of the 
private method.
   
   if (! isNotePathUnused(newNotePath) {
  throw new IOException("Note '" + notePath + "' is already in use");
   }
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

cuspymd commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611663144



##
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, 
$location, $rootScope,
   $scope.updateNoteName = function(newName) {
 const trimmedNewName = newName.trim();
 if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+  $scope.note.oldName = $scope.note.name;
   $scope.note.name = trimmedNewName;
   websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
 }
   };
 
+  $scope.$on('setNoteMenu', function(event, notes) {
+$scope.note.oldName = undefined;
+  });
+
+  $scope.$on('errorInfo', function(event, notes) {
+if ($scope.note.oldName !== undefined) {
+  $scope.note.name = $scope.note.oldName;
+  $scope.note.oldName = undefined;
+}
+  });

Review comment:
   Couldn't other errors occur while `renameNote()` is in progress?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4091: [ZEPPELIN-5319] Incorrect markdown in the documentation of sap interpreter

[GitBox]

Reamer commented on pull request #4091:
URL: https://github.com/apache/zeppelin/pull/4091#issuecomment-817857107


   LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

Reamer commented on pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072#issuecomment-817859179


   I will merge this PR into master and branch-0.9 on Thursday if no further 
comments are received.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r611713414



##
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, 
$location, $rootScope,
   $scope.updateNoteName = function(newName) {
 const trimmedNewName = newName.trim();
 if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+  $scope.note.oldName = $scope.note.name;
   $scope.note.name = trimmedNewName;
   websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
 }
   };
 
+  $scope.$on('setNoteMenu', function(event, notes) {
+$scope.note.oldName = undefined;
+  });
+
+  $scope.$on('errorInfo', function(event, notes) {
+if ($scope.note.oldName !== undefined) {
+  $scope.note.name = $scope.note.oldName;
+  $scope.note.oldName = undefined;
+}
+  });

Review comment:
   Yes, I can think of 2 alternatives:
   - Return a specific error response for the rename action
   - Push a `NOTES_INFO` update in the error case to update the UI
   
   What are you thoughts on the 2 approaches?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-817891494


   > I see that you have made changes in the `zeppelin-web` submodule. We a 
newer frontend submodule `zeppelin-web-angular`. Unfortunately, I am not a 
NodeJS developer, so I cannot check these parts. Why don't you have any changes 
in `zeppelin-web-angular`?
   
   I ran the new UI using the README and in the browser the UI looked very 
similar, and my changes were there. I assumed that this component was still 
using the old code, however I will take another look.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-817891879


   Looks like I have a flaky UI test, will address


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612252990



##
File path: zeppelin-web/src/app/notebook/notebook.controller.js
##
@@ -539,11 +539,23 @@ function NotebookCtrl($scope, $route, $routeParams, 
$location, $rootScope,
   $scope.updateNoteName = function(newName) {
 const trimmedNewName = newName.trim();
 if (trimmedNewName.length > 0 && $scope.note.name !== trimmedNewName) {
+  $scope.note.oldName = $scope.note.name;
   $scope.note.name = trimmedNewName;
   websocketMsgSrv.renameNote($scope.note.id, $scope.note.name, true);
 }
   };
 
+  $scope.$on('setNoteMenu', function(event, notes) {
+$scope.note.oldName = undefined;
+  });
+
+  $scope.$on('errorInfo', function(event, notes) {
+if ($scope.note.oldName !== undefined) {
+  $scope.note.name = $scope.note.oldName;
+  $scope.note.oldName = undefined;
+}
+  });

Review comment:
   I went with a combined approach:
   - Define an explicit exception for this error
   - Push a note update message to refresh stale UI




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#issuecomment-818563782


   I have reworked this change to be server side only, no UI changes required 
now


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

Reamer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612301146



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##
@@ -378,6 +386,24 @@ private String getNoteName(String notePath) {
 return notePath.substring(pos + 1);
   }
 
+  private boolean validateNotePathDoesNotExist(String notePath) {

Review comment:
   
   
   Just a small style change.
   Please rename your method name to improve readability.
   https://rules.sonarsource.com/java/tag/convention/RSPEC-2047
   
   btw. +1 for the new Exception




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Bowen0729 opened a new pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

Bowen0729 opened a new pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093


   ### What is this PR for?
   Add Feature 'delete paragraph' for ZeppelinClient
   
   ### What type of PR is it?
   [Improvement]
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/ZEPPELIN-5322
   
   ### How should this be tested?
   CI
   unit test
   
   ### Questions:
   * Does the licenses files need update? no 
   * Is there breaking changes for older versions? no 
   * Does this needs documentation? no
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] dannycranmer commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

dannycranmer commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612393692



##
File path: 
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/NoteManager.java
##
@@ -378,6 +386,24 @@ private String getNoteName(String notePath) {
 return notePath.substring(pos + 1);
   }
 
+  private boolean validateNotePathDoesNotExist(String notePath) {

Review comment:
   I have renamed inline with the feedback. Thanks




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4092: [ZEPPELIN-4506] Check for duplicate note on rename of a notebook

[GitBox]

cuspymd commented on a change in pull request #4092:
URL: https://github.com/apache/zeppelin/pull/4092#discussion_r612500675



##
File path: 
zeppelin-server/src/test/java/org/apache/zeppelin/service/NotebookServiceTest.java
##
@@ -331,6 +333,26 @@ public void testNoteOperations() throws IOException {
 assertEquals(0, notesInfo.size());
   }
 
+  @Test
+  public void testRenameNoteRejectsDuplicate() throws IOException {
+Note note1 = notebookService.createNote("/folder/note1", "test", true, 
context, callback);
+assertEquals("note1", note1.getName());
+verify(callback).onSuccess(note1, context);
+
+reset(callback);
+Note note2 = notebookService.createNote("/folder/note2", "test", true, 
context, callback);
+assertEquals("note2", note2.getName());
+verify(callback).onSuccess(note2, context);
+
+reset(callback);
+ArgumentCaptor exception = 
ArgumentCaptor.forClass(NotePathAlreadyExistsException.class);
+notebookService.renameNote(note1.getId(), "/folder/note2", false, context, 
callback);
+verify(callback).onFailure(exception.capture(), any(ServiceContext.class));

Review comment:
   It would have been better to check that the newly added 
`broadcastNote()` call part works properly when the error occurs through the 
`NotebookServer::onMessage()` method test, but the test for that part seems 
quite complicated.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd opened a new pull request #4094: [ZEPPELIN-5321] Missing links of some interpreters in index page and link menu

[GitBox]

cuspymd opened a new pull request #4094:
URL: https://github.com/apache/zeppelin/pull/4094


   ### What is this PR for?
   Fix missing interpreter links in document page
   
   ### What type of PR is it?
   [Documentation]
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5321
   
   ### How should this be tested?
   * Check updated document locally
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4094: [ZEPPELIN-5321] Missing links of some interpreters in index page and link menu

[GitBox]

zjffdu commented on pull request #4094:
URL: https://github.com/apache/zeppelin/pull/4094#issuecomment-819176499


   Thanks @cuspymd for the contribution, LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

cuspymd commented on a change in pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093#discussion_r612913260



##
File path: 
zeppelin-client/src/main/java/org/apache/zeppelin/client/ZeppelinClient.java
##
@@ -559,6 +559,24 @@ public String addParagraph(String noteId, String title, 
String text) throws Exce
 return jsonNode.getObject().getString("body");
   }
 
+  /**
+   * delete paragraph with noteId and paragraphID
+   *
+   * @param noteId
+   * @param paragraphId
+   * @throws Exception

Review comment:
   It would be nice if there were more helpful descriptions. Of course 
you've referenced the existing code, but isn't there a need to copy the 
existing bad parts?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Bowen0729 commented on pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

Bowen0729 commented on pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093#issuecomment-819272541


   > It would be nice if there were more helpful descriptions. Of course you've 
referenced the existing code, but isn't there a need to copy the existing bad 
parts?
   
   Sure, you're right.
   And I found other method to solve my problem, this pr is not that necessary.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Bowen0729 closed pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

Bowen0729 closed pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Bowen0729 commented on pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

Bowen0729 commented on pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093#issuecomment-819273546


   @zjffdu should I close this pr or merge it after review?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

zjffdu commented on pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093#issuecomment-819288928


   @Bowen0729 If this PR is not needed, you can close it an its jira ticket as 
well.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] Bowen0729 closed pull request #4093: [ZEPPELIN-55322] Add Feature 'delete paragraph' for ZeppelinClient

[GitBox]

Bowen0729 closed pull request #4093:
URL: https://github.com/apache/zeppelin/pull/4093


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd opened a new pull request #4095: [ZEPPELIN-5325] Broken table column at yarn.html

[GitBox]

cuspymd opened a new pull request #4095:
URL: https://github.com/apache/zeppelin/pull/4095


   ### What is this PR for?
   Add missing column of table in yarn document
   
   ### What type of PR is it?
   [Documentation]
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5325
   
   ### How should this be tested?
   * Check updated document locally
   
   ### Screenshots (if appropriate)
   
![image](https://user-images.githubusercontent.com/8870299/114742372-a38c6e00-9d86-11eb-8793-6740fd14f1fa.png)
   
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4072: [ZEPPELIN-4983] Download local repo to interpreter nodes

[GitBox]

asfgit closed pull request #4072:
URL: https://github.com/apache/zeppelin/pull/4072


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu opened a new pull request #4096: [HOTFIX] CI failed due to dns reverse lookup issue

[GitBox]

zjffdu opened a new pull request #4096:
URL: https://github.com/apache/zeppelin/pull/4096


   ### What is this PR for?
   
   This is a hotfix to fix the ci failure due to dns reverse lookup issue. Some 
of the code is copied from pulsar project.
   
https://github.com/lhotari/pulsar/commit/b4300db307f542d2ece7ad9f487f078d00210a27
   
   
   ### What type of PR is it?
   [ Hot Fix ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * 
   
   ### 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4096: [HOTFIX] CI failed due to dns reverse lookup issue

[GitBox]

cuspymd commented on a change in pull request #4096:
URL: https://github.com/apache/zeppelin/pull/4096#discussion_r615405020



##
File path: 
spark/interpreter/src/test/java/org/apache/zeppelin/spark/SparkShinyInterpreterTest.java
##
@@ -102,7 +102,7 @@ public void testSparkShinyApp() throws IOException, 
InterpreterException, Interr
 Thread.sleep(5 * 1000);
 // extract shiny url
 List resultMessages = 
context2.out.toInterpreterResultMessage();
-assertEquals(1, resultMessages.size());
+assertEquals(resultMessages + "", 1, resultMessages.size());

Review comment:
   The result seems be the same as `resultMessages.toString()`, but 
wouldn't it be better to use a consistent expression with other code?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4096: [HOTFIX] CI failed due to dns reverse lookup issue

[GitBox]

zjffdu commented on a change in pull request #4096:
URL: https://github.com/apache/zeppelin/pull/4096#discussion_r615417651



##
File path: 
spark/interpreter/src/test/java/org/apache/zeppelin/spark/SparkShinyInterpreterTest.java
##
@@ -102,7 +102,7 @@ public void testSparkShinyApp() throws IOException, 
InterpreterException, Interr
 Thread.sleep(5 * 1000);
 // extract shiny url
 List resultMessages = 
context2.out.toInterpreterResultMessage();
-assertEquals(1, resultMessages.size());
+assertEquals(resultMessages + "", 1, resultMessages.size());

Review comment:
   This change is only for diagnose to display the content of 
resultMessages when this assertion fails. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4096: [HOTFIX] CI failed due to dns reverse lookup issue

[GitBox]

zjffdu commented on a change in pull request #4096:
URL: https://github.com/apache/zeppelin/pull/4096#discussion_r615421141



##
File path: 
spark/interpreter/src/test/java/org/apache/zeppelin/spark/SparkShinyInterpreterTest.java
##
@@ -102,7 +102,7 @@ public void testSparkShinyApp() throws IOException, 
InterpreterException, Interr
 Thread.sleep(5 * 1000);
 // extract shiny url
 List resultMessages = 
context2.out.toInterpreterResultMessage();
-assertEquals(1, resultMessages.size());
+assertEquals(resultMessages + "", 1, resultMessages.size());

Review comment:
   > The result seems be the same as `resultMessages.toString()`, but 
wouldn't it be better to use a consistent expression with other code?
   
   Right, let me update it 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org