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/09/22 07:25:46 UTC
[GitHub] [zeppelin] zjffdu opened a new pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
zjffdu opened a new pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917
### What is this PR for?
This PR add method `init` to pass `ZeppelinConfiguration` in the thrift interface between zeppelin server and interpreter process. And put lifecycle manager initialization in the init method.
### What type of PR is it?
[Bug Fix ]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5053
### How should this be 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:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495763570
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
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] zjffdu commented on pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-699572216
@Reamer CI is passed，could you help review and verify it on k8s ?
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495659543
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
What typo do you mean ?
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495749909
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
Oops，let me fix 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:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-698905250
When this is ready for review, let me know.
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-699854926
> @Reamer CI is passed，could you help review and verify it on k8s ?
I will test this on K8s today.
----------------------------------------------------------------
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 edited a comment on pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer edited a comment on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700032307
> > @Reamer CI is passed，could you help review and verify it on k8s ?
>
> I will test this on K8s today.
Hi @zjffdu,
I have tested it on K8s. Unfortunately it does not work.
```
INFO [2020-09-28 15:51:07,059] ({pool-3-thread-2} RemoteInterpreterServer.java[createLifecycleManager]:374) - Creating interpreter lifecycle manager: org.apache.zeppelin.interpreter.lifecycle.NullLifecycleManager
```
I have found the reason for this. At the moment I have configured Zeppelin only via the environment variable, as described in [our example](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L17-L38). If you have no configuration file, `properties` are empty.
I see two ways to solve this problem.
- Load the same environment key value map into the interpreter pod -> very easy and secure. Just copy this [part](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L143-L145) into [interpreter-spec.yaml](https://github.com/apache/zeppelin/blob/master/k8s/interpreter/100-interpreter-spec.yaml)
- Filling properties during startup in the Zeppelin server with values from environment
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700573319
> Thanks @Reamer for the testing, I would prefer the second approach. Because I think this not only happens in k8s, in other modes it also possible to define configuration via env.
You are right, we should also find a solution for other modes.
> @Reamer I have fixed that in another commit, please help try that in k8s.
Now it works on K8s with an environment configuration setup.
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-698905250
When this is ready for review, let me know.
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700032307
> > @Reamer CI is passed，could you help review and verify it on k8s ?
>
> I will test this on K8s today.
Hi @zjffdu,
I have tested it on K8s. Unfortunately it does not work.
```
INFO [2020-09-28 15:51:07,059] ({pool-3-thread-2} RemoteInterpreterServer.java[createLifecycleManager]:374) - Creating interpreter lifecycle manager: org.apache.zeppelin.interpreter.lifecycle.NullLifecycleManager
```
I have found the reason for this. At the moment I have configured Zeppelin only via the environment variable, as described in [our example](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L17-L38). If you have no configuration file `properties` are empty.
I see two ways to solve this problem.
- Load the same environment key value map into the interpreter pod -> very easy and secure. Just copy this [part](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L143-L145) into [interpreter-spec.yaml](https://github.com/apache/zeppelin/blob/master/k8s/interpreter/100-interpreter-spec.yaml)
- Filling properties during startup in the Zeppelin server
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700121613
Thanks @Reamer for the testing, I would prefer the second approach. Because I think this not only happens in k8s, in other modes it also possible to define configuration via env.
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495747898
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
double function call:
`((RemoteInterpreter) interpreter).internal_create();` is called twice
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r492531084
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
Typo?
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495813140
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -184,12 +185,18 @@ public static synchronized ZeppelinConfiguration create() {
return conf;
}
+ public Map<String, String> getProperties() {
+ return this.properties;
+ }
+
public static void reset() {
conf = null;
}
public void setProperty(String name, String value) {
- this.properties.put(name, value);
+ if (name != null && value != null) {
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] zjffdu commented on pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-702723341
Will merge it 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:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r492531084
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
Typo?
----------------------------------------------------------------
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 pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700123339
@Reamer I have fixed that in another commit, please help try that in k8s.
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495749909
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java
##########
@@ -123,6 +123,7 @@ public void open() throws InterpreterException {
try {
if (!(interpreter instanceof ConfInterpreter)) {
((RemoteInterpreter) interpreter).internal_create();
+ ((RemoteInterpreter) interpreter).internal_create();
Review comment:
Oops，
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495811761
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -76,14 +76,15 @@ private void initProperties() {
for (ConfigurationNode p : nodes) {
String name = (String) p.getChildren("name").get(0).getValue();
String value = (String) p.getChildren("value").get(0).getValue();
- if (!StringUtils.isEmpty(name)) {
+ if (!StringUtils.isEmpty(name) && !StringUtils.isEmpty(value)) {
Review comment:
Good point, 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 edited a comment on pull request #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer edited a comment on pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#issuecomment-700032307
> > @Reamer CI is passed，could you help review and verify it on k8s ?
>
> I will test this on K8s today.
Hi @zjffdu,
I have tested it on K8s. Unfortunately it does not work.
```
INFO [2020-09-28 15:51:07,059] ({pool-3-thread-2} RemoteInterpreterServer.java[createLifecycleManager]:374) - Creating interpreter lifecycle manager: org.apache.zeppelin.interpreter.lifecycle.NullLifecycleManager
```
I have found the reason for this. At the moment I have configured Zeppelin only via the environment variable, as described in [our example](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L17-L38). If you have no configuration file, `properties` are empty.
I see two ways to solve this problem.
- Load the same environment key value map into the interpreter pod -> very easy and secure. Just copy this [part](https://github.com/apache/zeppelin/blob/536e66ce76aaefabeb3784bdc8013f7a2513d7f0/k8s/zeppelin-server.yaml#L143-L145) into [interpreter-spec.yaml](https://github.com/apache/zeppelin/blob/master/k8s/interpreter/100-interpreter-spec.yaml)
- Filling properties during startup in the Zeppelin server
----------------------------------------------------------------
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 #3917: [ZEPPELIN-5053]. Transfer Zeppelin server configuration to a remote interpreter
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #3917:
URL: https://github.com/apache/zeppelin/pull/3917#discussion_r495767500
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -184,12 +185,18 @@ public static synchronized ZeppelinConfiguration create() {
return conf;
}
+ public Map<String, String> getProperties() {
+ return this.properties;
+ }
+
public static void reset() {
conf = null;
}
public void setProperty(String name, String value) {
- this.properties.put(name, value);
+ if (name != null && value != null) {
Review comment:
Maybe `StringUtils.isNoneBlank()` is better in this place, because with this code you can add keys and/or values like `" "`.
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -76,14 +76,15 @@ private void initProperties() {
for (ConfigurationNode p : nodes) {
String name = (String) p.getChildren("name").get(0).getValue();
String value = (String) p.getChildren("value").get(0).getValue();
- if (!StringUtils.isEmpty(name)) {
+ if (!StringUtils.isEmpty(name) && !StringUtils.isEmpty(value)) {
Review comment:
Here I would also prefer `StringUtils.isNoneBlank()`.
----------------------------------------------------------------
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