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 2021/09/15 06:32:04 UTC
[GitHub] [zeppelin] jongyoul opened a new pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
jongyoul opened a new pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223
### What is this PR for?
Zeppelin uses REST and Websocket but the stack of Websocket depends on Jetty itself and it also has a different stack for it. I would like to get rid of Jetty's dependency of Websocket and merge REST and Websocket within a single stack.
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Remove Jetty's Websocket dependencies
* [ ] - Remove `NotebookSocket`
* [ ] - Handle `javax.websocket.Session` directly instead of using `NotebookSocket`
* [ ] - Merge them into single DI framework without ServiceLocator of hk2 <- Not sure if it's possible yet.
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5518
### How should this be tested?
* Test cases should be passed
### 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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r715309298
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
Review comment:
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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r715307731
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
+public class SessionConfigurator extends ServerEndpointConfig.Configurator {
+ @Override
+ public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request,
+ HandshakeResponse response) {
+ List<String> holder;
+ holder = request.getHeaders().get(WatcherSecurityKey.HTTP_HEADER);
+ sec.getUserProperties().put(WatcherSecurityKey.HTTP_HEADER,
+ null != holder && holder.size() > 0 ? holder.get(0) : 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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r710229365
##########
File path: pom.xml
##########
@@ -589,6 +590,12 @@
</exclusions>
</dependency>
+ <dependency>
Review comment:
I'm fixing not to set dependency management block on pom.xml directly. :-)
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711175060
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
It could be but Pegdown uses `asm` pages inside it but it's related to the implementation by Jetty. So without it, tests wouldn't be passed properly.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r714504151
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
+public class SessionConfigurator extends ServerEndpointConfig.Configurator {
+ @Override
+ public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request,
+ HandshakeResponse response) {
+ List<String> holder;
+ holder = request.getHeaders().get(WatcherSecurityKey.HTTP_HEADER);
+ sec.getUserProperties().put(WatcherSecurityKey.HTTP_HEADER,
+ null != holder && holder.size() > 0 ? holder.get(0) : null);
Review comment:
Just a minor code smell.
Collection.isEmpty() should be used to test for emptiness. Take a look into https://rules.sonarsource.com/java/RSPEC-1155
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
Review comment:
A little documentation as to why this is necessary would be good.
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
+public class SessionConfigurator extends ServerEndpointConfig.Configurator {
+ @Override
+ public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request,
+ HandshakeResponse response) {
+ List<String> holder;
+ holder = request.getHeaders().get(WatcherSecurityKey.HTTP_HEADER);
+ sec.getUserProperties().put(WatcherSecurityKey.HTTP_HEADER,
+ null != holder && holder.size() > 0 ? holder.get(0) : null);
+ holder = request.getHeaders().get(CorsUtils.HEADER_ORIGIN);
+ sec.getUserProperties().put(CorsUtils.HEADER_ORIGIN,
+ null != holder && holder.size() > 0 ? holder.get(0) : null);
Review comment:
Same as above
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711905439
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
I have prepared something. #4229
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711986085
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
Yeap. I think it should be handled as well. By the way, it looks like having redundant dependencies on `zeppelin-jupyter` by having `markdown`. `JupyterUtil` parser markdown paragraph but I don't think we don't have to depend on the whole `markdown` module for that case. I think we'd better decouple them and copy a small piece of code. #4228
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711899941
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
I think we should remove the pegdown parser from the md interpreter.
According to [pegdown README](https://github.com/sirthias/pegdown/blob/master/README.markdown), pegdown has reached the end of its life a long time ago.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711572935
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
I have thought about your suggestion and agreed that it should be handled at first. https://issues.apache.org/jira/browse/ZEPPELIN-5527
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#issuecomment-925511051
I'll divide tasks into several issues. This PR only remove Jetty's websocket stack. I'll change `NotebookSocket` with another PR. I'll merge it if there's no further discussion.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711171057
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
Should put in another PR ?
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r710161557
##########
File path: pom.xml
##########
@@ -589,6 +590,12 @@
</exclusions>
</dependency>
+ <dependency>
Review comment:
Basically, I agree with you but it's related to `markdown` and `zeppelin-server`. In my understanding, it needs to add here to affect them in my understanding.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711175060
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
It could be but `Pegdown` uses `asm` pages inside it but it's related to the implementation by Jetty. So without it, tests wouldn't be passed properly.
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
It could be but `Pegdown` uses `asm` package inside it but it's related to the implementation by Jetty. So without it, tests wouldn't be passed properly.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711175630
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
I also think it needs to remove `markdown` dependency from `jupyter-support` and `zeppelin-zengine`. I'll prepare for a new PR to handle it officially.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r711175630
##########
File path: zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
##########
@@ -78,7 +78,7 @@ public JupyterUtil() {
.registerSubtype(ExecuteResult.class, "execute_result")
.registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
.registerSubtype(Error.class, "error");
- this.markdownProcessor = new PegdownParser();
+ this.markdownProcessor = new FlexmarkParser();
Review comment:
I also think it need to remove `markdown` dependency from `jupyter-support` and `zeppelin-zengine`.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r710161557
##########
File path: pom.xml
##########
@@ -589,6 +590,12 @@
</exclusions>
</dependency>
+ <dependency>
Review comment:
Basically, I agree with you but it's related to `markdown` and `zeppelin-server`. In my understanding, it needs to add dependency management blocks here to affect them in my understanding.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r715307790
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/SessionConfigurator.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.socket;
+
+import java.util.List;
+
+import javax.websocket.HandshakeResponse;
+import javax.websocket.server.HandshakeRequest;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.apache.zeppelin.server.ZeppelinServer;
+import org.apache.zeppelin.util.WatcherSecurityKey;
+import org.apache.zeppelin.utils.CorsUtils;
+
+public class SessionConfigurator extends ServerEndpointConfig.Configurator {
+ @Override
+ public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request,
+ HandshakeResponse response) {
+ List<String> holder;
+ holder = request.getHeaders().get(WatcherSecurityKey.HTTP_HEADER);
+ sec.getUserProperties().put(WatcherSecurityKey.HTTP_HEADER,
+ null != holder && holder.size() > 0 ? holder.get(0) : null);
+ holder = request.getHeaders().get(CorsUtils.HEADER_ORIGIN);
+ sec.getUserProperties().put(CorsUtils.HEADER_ORIGIN,
+ null != holder && holder.size() > 0 ? holder.get(0) : 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.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul closed pull request #4223: [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul closed pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r710154902
##########
File path: pom.xml
##########
@@ -589,6 +590,12 @@
</exclusions>
</dependency>
+ <dependency>
Review comment:
I would place this dependency management in the "zeppelin-server/pom.xml" to keep the parent pom.xml clear.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#issuecomment-921904083
Yes, it could be but, wothout it, test would be passed. WDYT?
2021년 9월 18일 (토) 00:52, Jeff Zhang ***@***.***>님이 작성:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
> <https://github.com/apache/zeppelin/pull/4223#discussion_r711171057>:
>
> > @@ -78,7 +78,7 @@ public JupyterUtil() {
> .registerSubtype(ExecuteResult.class, "execute_result")
> .registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
> .registerSubtype(Error.class, "error");
> - this.markdownProcessor = new PegdownParser();
> + this.markdownProcessor = new FlexmarkParser();
>
> Should put in another PR ?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/zeppelin/pull/4223#pullrequestreview-757619597>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AA3R7FWTSQNVU5QRJP773HDUCNP2TANCNFSM5EBWG5DQ>
> .
>
--
이종열, Jongyoul Lee, 李宗烈
http://madeng.net
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul removed a comment on pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul removed a comment on pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#issuecomment-921904083
Yes, it could be but, wothout it, test would be passed. WDYT?
2021년 9월 18일 (토) 00:52, Jeff Zhang ***@***.***>님이 작성:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> zeppelin-jupyter/src/main/java/org/apache/zeppelin/jupyter/JupyterUtil.java
> <https://github.com/apache/zeppelin/pull/4223#discussion_r711171057>:
>
> > @@ -78,7 +78,7 @@ public JupyterUtil() {
> .registerSubtype(ExecuteResult.class, "execute_result")
> .registerSubtype(DisplayData.class, "display_data").registerSubtype(Stream.class, "stream")
> .registerSubtype(Error.class, "error");
> - this.markdownProcessor = new PegdownParser();
> + this.markdownProcessor = new FlexmarkParser();
>
> Should put in another PR ?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/zeppelin/pull/4223#pullrequestreview-757619597>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AA3R7FWTSQNVU5QRJP773HDUCNP2TANCNFSM5EBWG5DQ>
> .
>
--
이종열, Jongyoul Lee, 李宗烈
http://madeng.net
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4223: [WIP] [ZEPPELIN-5518] Adopt JSR356 for Websocket
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4223:
URL: https://github.com/apache/zeppelin/pull/4223#discussion_r710161557
##########
File path: pom.xml
##########
@@ -589,6 +590,12 @@
</exclusions>
</dependency>
+ <dependency>
Review comment:
Basically, I agree with it but it's related to `markdown` and `zeppelin-server`. In my understanding, it needs to add here to affect them in my understanding.
--
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-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org