You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2020/06/04 07:50:49 UTC
[GitHub] [zeppelin] zjffdu opened a new pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
zjffdu opened a new pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785
### What is this PR for?
This is a trivial PR which add `getInterpreterGroupId` to `InterpreterClient`, and use `interpreterGroupId` as the identifier of RemoteInterpreterProcess because `interpreterGroupId` is also unique identifier of InterpreterGroup
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-4852
### 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:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785#discussion_r435142920
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java
##########
@@ -141,7 +141,7 @@ public void start(String userName) throws IOException {
public void stop() {
if (isRunning()) {
- LOGGER.info("Kill interpreter process");
+ LOGGER.info("Kill interpreter process for interpreter group: " + getInterpreterGroupId());
Review comment:
We should avoid the creation of objects if a logging level is disabled. String concatenation creates a new String object.
This particular line should change to
```
LOGGER.info("Kill interpreter process for interpreter group: {}", getInterpreterGroupId());
```
----------------------------------------------------------------
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
[GitHub] [zeppelin] Reamer commented on a change in pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785#discussion_r435142920
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java
##########
@@ -141,7 +141,7 @@ public void start(String userName) throws IOException {
public void stop() {
if (isRunning()) {
- LOGGER.info("Kill interpreter process");
+ LOGGER.info("Kill interpreter process for interpreter group: " + getInterpreterGroupId());
Review comment:
We should avoid the creation of objects if a logging level is disabled
This particular line should change to
```
LOGGER.info("Kill interpreter process for interpreter group: {}", getInterpreterGroupId());
```
----------------------------------------------------------------
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
[GitHub] [zeppelin] asfgit closed pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785
----------------------------------------------------------------
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
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785#discussion_r435070208
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterRunningProcess.java
##########
@@ -80,6 +88,10 @@ public Void call(RemoteInterpreterService.Client client) throws Exception {
} catch (Exception e) {
logger.warn("ignore the exception when shutting down interpreter process.", e);
}
+
+ // Shutdown connection
+ shutdown();
+ logger.info("Remote process of interpreter group: " + getInterpreterGroupId() + " is terminated");
Review comment:
fixed
----------------------------------------------------------------
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
[GitHub] [zeppelin] Reamer commented on a change in pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785#discussion_r435142920
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterManagedProcess.java
##########
@@ -141,7 +141,7 @@ public void start(String userName) throws IOException {
public void stop() {
if (isRunning()) {
- LOGGER.info("Kill interpreter process");
+ LOGGER.info("Kill interpreter process for interpreter group: " + getInterpreterGroupId());
Review comment:
We should avoid the creation of objects if a logging level is disabled
This particular line should change to
```
LOGGER.info("Kill interpreter process for interpreter group: {}", getInterpreterGroupId());
```
String concatenation creates a new String object.
----------------------------------------------------------------
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
[GitHub] [zeppelin] Reamer commented on a change in pull request #3785: [ZEPPELIN-4852]. Add name to RemoteInterpreterProcess
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3785:
URL: https://github.com/apache/zeppelin/pull/3785#discussion_r435066518
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterRunningProcess.java
##########
@@ -80,6 +88,10 @@ public Void call(RemoteInterpreterService.Client client) throws Exception {
} catch (Exception e) {
logger.warn("ignore the exception when shutting down interpreter process.", e);
}
+
+ // Shutdown connection
+ shutdown();
+ logger.info("Remote process of interpreter group: " + getInterpreterGroupId() + " is terminated");
Review comment:
Could you please make the logger 'public static final' and avoid the creation of objects if the logger is disabled for the INFO level ( `LOGGER.info(String format, Object... 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:
users@infra.apache.org