[GitHub] [zeppelin] Reamer commented on a change in pull request #3925: [ZEPPELIN-5070] Improve start/shutdown and signal handling

[GitBox <gi...@apache.org>]

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



##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/launcher/SparkInterpreterLauncher.java
##########
@@ -171,7 +171,7 @@ public SparkInterpreterLauncher(ZeppelinConfiguration zConf, RecoveryStorage rec
       sparkConfBuilder.append(" --proxy-user " + context.getUserName());
     }
 
-    env.put("ZEPPELIN_SPARK_CONF", escapeSpecialCharacter(sparkConfBuilder.toString()));
+    env.put("ZEPPELIN_SPARK_CONF", sparkConfBuilder.toString());

Review comment:
       Escaping 'ZEPPELIN_SPARK_CONF' is no longer necessary, because all values are passed to 'exec' as string arguments. The bash escaped string values automatically.
   Because of `exec` the bash process is replaced by the jvm process, so it is not possible to insert further commands after `exec`.
   
   What do you think?

##########
File path: rlang/src/test/java/org/apache/zeppelin/r/ShinyInterpreterTest.java
##########
@@ -221,7 +221,7 @@ public void testInvalidShinyApp()
 
     resultMessages = context2.out.toInterpreterResultMessage();
     assertTrue(resultMessages.get(1).getData(),
-            resultMessages.get(1).getData().contains("object 'Invalid_code' not found"));
+            resultMessages.get(1).getData().contains("'Invalid_code'"));

Review comment:
       My system language is German. When I run tests locally, some tests fail because the JVM translates the error message into German. With commit https://github.com/apache/zeppelin/pull/3925/commits/ea099abef02a0b971c863c1bb1b70aa149bb74fc I have removed the part that is translated by the JVM.

##########
File path: python/src/test/java/org/apache/zeppelin/python/IPythonInterpreterTest.java
##########
@@ -475,7 +475,7 @@ public void testIPythonFailToLaunch() throws InterpreterException {
       fail("Should not be able to start IPythonInterpreter");
     } catch (InterpreterException e) {
       String exceptionMsg = ExceptionUtils.getStackTrace(e);
-      assertTrue(exceptionMsg, exceptionMsg.contains("No such file or directory"));
+      assertTrue(exceptionMsg, exceptionMsg.contains("java.io.IOException"));

Review comment:
       My system language is German. When I run tests locally, some tests fail because the JVM translates the error message into German. With commit https://github.com/apache/zeppelin/pull/3925/commits/ea099abef02a0b971c863c1bb1b70aa149bb74fc I have removed the part that is translated by the JVM.

##########
File path: zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
##########
@@ -1232,7 +1234,7 @@ public void testPerSessionInterpreterCloseOnNoteRemoval() throws IOException, In
       setting.getOption().setPerNote(setting.getOption().SCOPED);
       notebook.getInterpreterSettingManager().restart(setting.getId());
     }
-
+    Thread.sleep(2000);

Review comment:
       I add this lines during debugging because of failed tests. But this was before adding this [magic line](https://github.com/apache/zeppelin/pull/3925/files#diff-87d8954289365d4fc9ab53cd9b2257ac28cbdef284ec83565e4d5bf9bafc6eedR248), which removes the shutdownhook thread. I'll try to revert the sleep statements.
   
   I added these lines during debugging because of failed tests. But that was before adding this [magic line](https://github.com/apache/zeppelin/pull/3925/files#diff-87d8954289365d4fc9ab53cd9b2257ac28cbdef284ec83565e4d5bf9bafc6eedR248) that removes the shutdownhook thread in case of a regular shutdown. I will try to remove the sleep statements.

##########
File path: zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
##########
@@ -1232,7 +1234,7 @@ public void testPerSessionInterpreterCloseOnNoteRemoval() throws IOException, In
       setting.getOption().setPerNote(setting.getOption().SCOPED);
       notebook.getInterpreterSettingManager().restart(setting.getId());
     }
-
+    Thread.sleep(2000);

Review comment:
       I added these lines during debugging because of failed tests. But that was before adding this [magic line](https://github.com/apache/zeppelin/pull/3925/files#diff-87d8954289365d4fc9ab53cd9b2257ac28cbdef284ec83565e4d5bf9bafc6eedR248) that removes the shutdownhook thread in case of a regular shutdown. I will try to remove the sleep statements.

##########
File path: zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java
##########
@@ -1232,7 +1234,7 @@ public void testPerSessionInterpreterCloseOnNoteRemoval() throws IOException, In
       setting.getOption().setPerNote(setting.getOption().SCOPED);
       notebook.getInterpreterSettingManager().restart(setting.getId());
     }
-
+    Thread.sleep(2000);

Review comment:
       I have removed the Sleep Statements.




----------------------------------------------------------------
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:
users@infra.apache.org