[GitHub] [zeppelin] rickchengx commented on pull request #4174: [ZEPPELIN-5442] Allow users to set a separate namespace for the interpreter
rickchengx commented on pull request #4174: URL: https://github.com/apache/zeppelin/pull/4174#issuecomment-895749927 Hi, @Reamer Thanks for the comments. I have moved the common functions (`isRunningOnKubernetes()`, `readFile()`, `getInterpreterNamespace()` and `getCurrentK8sNamespace()`) into `org.apache.zeppelin.interpreter.launcher.K8sUtils` to avoid duplicate code. And I also make it more clear to distinguish the namespace of server and 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4197: [ZEPPELIN-5478] Update flink version
zjffdu commented on pull request #4197: URL: https://github.com/apache/zeppelin/pull/4197#issuecomment-895688893 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4199: [ZEPPELIN-5481] Support to run R interpreter in yarn mode with customized conda env
zjffdu commented on pull request #4199: URL: https://github.com/apache/zeppelin/pull/4199#issuecomment-895688812 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4198: [ZEPPELIN-5479] %python.sql doesn't work with ipython interpreter
zjffdu commented on pull request #4198: URL: https://github.com/apache/zeppelin/pull/4198#issuecomment-895688714 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] rickchengx commented on a change in pull request #4174: [ZEPPELIN-5442] Allow users to set a separate namespace for the interpreter
rickchengx commented on a change in pull request #4174: URL: https://github.com/apache/zeppelin/pull/4174#discussion_r685626738 ## File path: k8s/zeppelin-server.yaml ## @@ -213,14 +213,15 @@ rules: resources: ["roles", "rolebindings"] verbs: ["bind", "create", "get", "update", "patch", "list", "delete", "watch"] --- -kind: RoleBinding +kind: ClusterRoleBinding apiVersion: rbac.authorization.k8s.io/v1 metadata: name: zeppelin-server-role-binding subjects: - kind: ServiceAccount name: zeppelin-server + namespace: default Review comment: Yes, the namespace of zeppelin server is not sure. But the `clusterrolebinding` needs to specify the namespace of the service account, which is applied by user when user is creating the zeppelin server in k8s. So if user does not apply the `zeppelin-server` in the default namespace, he also needs to modify this `namespace` field in the `clusterrolebinding`. -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] zjffdu edited a comment on pull request #4200: ZEPPELIN-1554: Upgrade jekyll
zjffdu edited a comment on pull request #4200: URL: https://github.com/apache/zeppelin/pull/4200#issuecomment-895279241 @epugh Sorry, I forgot to mention that you can check the [README.md](https://github.com/apache/zeppelin/tree/master/docs) for how to generate docs. Building docs via docker would be very easy. -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4200: ZEPPELIN-1554: Upgrade jekyll
zjffdu commented on pull request #4200: URL: https://github.com/apache/zeppelin/pull/4200#issuecomment-895279241 @epugh Sorry to mention that you can check the [README.md](https://github.com/apache/zeppelin/tree/master/docs) for how to generate docs. Building docs via docker would be very easy. -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] epugh opened a new pull request #4200: ZEPPELIN-1554: Upgrade jekyll
epugh opened a new pull request #4200: URL: https://github.com/apache/zeppelin/pull/4200 ### What is this PR for? I wanted to generate the `docs` site on my Mac, and found a bunch of old dependencies that needed updating. I don't know if this is related to ZEPPELIN-1554, which looks like there was some good work that is similar to what I've done. If you want, I could pull in some of the other fixes done in https://github.com/apache/zeppelin/pull/1577 in other PR's... First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html ### What type of PR is it? Bug Fix - I think this is right since we need updates to run Jekyll. ### Todos * [ ] - Task ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-1554 ### How should this be tested? * Strongly recommended: add automated unit tests for any new or changed behavior * Outline any manual steps to test the PR here. * I ran the `bundle exec jekyll serve --watch` command. ### 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4199: [ZEPPELIN-5481] Support to run R interpreter in yarn mode with customized conda env
Reamer commented on a change in pull request #4199:
URL: https://github.com/apache/zeppelin/pull/4199#discussion_r685169039
##
File path: rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java
##
@@ -152,6 +153,17 @@ protected void initIRKernel() throws IOException,
InterpreterException {
}
}
+ @Override
+ protected Map setupKernelEnv() throws IOException {
+Map envs = super.setupKernelEnv();
+String pathEnv = envs.getOrDefault("PATH", "");
+if (condaEnv != null) {
+ pathEnv = new File(".").getAbsolutePath() + "/" + condaEnv + "/bin:" +
pathEnv;
Review comment:
Looks weirder than I thought, but I think that should be correct.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4199: [ZEPPELIN-5481] Support to run R interpreter in yarn mode with customized conda env
zjffdu commented on a change in pull request #4199:
URL: https://github.com/apache/zeppelin/pull/4199#discussion_r685154342
##
File path: rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java
##
@@ -152,6 +153,17 @@ protected void initIRKernel() throws IOException,
InterpreterException {
}
}
+ @Override
+ protected Map setupKernelEnv() throws IOException {
+Map envs = super.setupKernelEnv();
+String pathEnv = envs.getOrDefault("PATH", "");
+if (condaEnv != null) {
+ pathEnv = new File(".").getAbsolutePath() + "/" + condaEnv + "/bin:" +
pathEnv;
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.
To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r685128717
##
File path:
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry =
TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket()))
{
+LOG.debug("{} message: invalid ticket {}", receivedMessage.op,
receivedMessage.ticket);
+return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
-} else {
- if (!receivedMessage.op.equals(OP.PING)) {
-conn.send(serializeMessage(new
Message(OP.SESSION_LOGOUT).put("info",
-"Your ticket is invalid possibly due to server restart. "
-+ "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
`receivedMessage.ticket` is empty or null at this time. I think that
this message makes no sense.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4183: [ZEPPELIN-5464] Zeppelin server needs to obtain the permission to create configmaps under k8s mode
asfgit closed pull request #4183: URL: https://github.com/apache/zeppelin/pull/4183 -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4174: [ZEPPELIN-5442] Allow users to set a separate namespace for the interpreter
Reamer commented on a change in pull request #4174:
URL: https://github.com/apache/zeppelin/pull/4174#discussion_r685015571
##
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sStandardInterpreterLauncher.java
##
@@ -95,9 +100,11 @@ String getHostname() {
*/
private String getZeppelinService(InterpreterLaunchContext context) throws
IOException {
if (isRunningOnKubernetes()) {
+ //The namespace of zeppelin server can only be read from
Config.KUBERNETES_NAMESPACE_PATH while it runs in k8s cluster, it may be
different from the namespace of interpreter
+ String serverNamespace = readFile(Config.KUBERNETES_NAMESPACE_PATH,
Charset.defaultCharset()).trim();
Review comment:
Please move this part to a separate function. Something like
`getServerNamespace()`.
##
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sStandardInterpreterLauncher.java
##
@@ -70,6 +70,11 @@ boolean isRunningOnKubernetes() {
* @throws IOException if namespace file could not be read
*/
String getNamespace() throws IOException {
+//namespace may be set from the interpreter properties
Review comment:
The `K8sStandardInterpreterLauncher` is executed only on the Zeppelin
server side.
Please rename this function to `getInterpreterNamespace()`.
##
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sRemoteInterpreterProcess.java
##
@@ -275,7 +275,7 @@ Properties getTemplateBindings(String userName) {
Properties k8sProperties = new Properties();
// k8s template properties
-k8sProperties.put("zeppelin.k8s.namespace", getNamespace());
+k8sProperties.put("zeppelin.k8s.interpreter.namespace", getNamespace());
Review comment:
I think this should be the place where you need to call another method,
something like `getInterpreterNamespace()`, where the default value is the
output of `getServerNamespace()`.
##
File path: k8s/zeppelin-server.yaml
##
@@ -213,14 +213,15 @@ rules:
resources: ["roles", "rolebindings"]
verbs: ["bind", "create", "get", "update", "patch", "list", "delete",
"watch"]
---
-kind: RoleBinding
+kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: zeppelin-server-role-binding
subjects:
- kind: ServiceAccount
name: zeppelin-server
+ namespace: default
Review comment:
It is not sure that the `zeppelin-server` ServiceAccount is part of the
`default` namespace.
##
File path:
zeppelin-plugins/launcher/k8s-standard/src/main/java/org/apache/zeppelin/interpreter/launcher/K8sRemoteInterpreterProcess.java
##
@@ -135,6 +139,43 @@ public String getNamespace() {
return namespace;
Review comment:
Rename this variable to `serverNamespace` and change the function name
to `getServerNamespace()`.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [zeppelin] Reamer closed pull request #4127: ZEPPELIN-5397 : Modified the interpreter.sh script to be able to parse SPARK_SUBMIT_OPTIONS
Reamer closed pull request #4127: URL: https://github.com/apache/zeppelin/pull/4127 -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4127: ZEPPELIN-5397 : Modified the interpreter.sh script to be able to parse SPARK_SUBMIT_OPTIONS
Reamer commented on pull request #4127: URL: https://github.com/apache/zeppelin/pull/4127#issuecomment-895027038 Closed in favor of #4173 -- 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. To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4199: [ZEPPELIN-5481] Support to run R interpreter in yarn mode with customized conda env
Reamer commented on a change in pull request #4199:
URL: https://github.com/apache/zeppelin/pull/4199#discussion_r684964985
##
File path: rlang/src/main/java/org/apache/zeppelin/r/IRInterpreter.java
##
@@ -152,6 +153,17 @@ protected void initIRKernel() throws IOException,
InterpreterException {
}
}
+ @Override
+ protected Map setupKernelEnv() throws IOException {
+Map envs = super.setupKernelEnv();
+String pathEnv = envs.getOrDefault("PATH", "");
+if (condaEnv != null) {
+ pathEnv = new File(".").getAbsolutePath() + "/" + condaEnv + "/bin:" +
pathEnv;
Review comment:
Please use `File.separator` and `File.pathSeparator` to build the `PATH`
environment variable.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
