KUAN-HSUN-LI commented on code in PR #852:
URL: https://github.com/apache/submarine/pull/852#discussion_r862481022
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmarineServer.java:
##########
@@ -248,14 +254,37 @@ private static Server
setupJettyServer(SubmarineConfiguration conf) {
}
private static void setupNotebookServer(WebAppContext webapp,
Review Comment:
why not replace the notebook server with websocket server?
##########
submarine-server/server-core/src/test/java/org/apache/submarine/server/websocket/EnvironmentWebsocketTest.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.submarine.server.websocket;
+
+import org.apache.submarine.server.AbstractSubmarineServerTest;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.StatusCode;
+import org.eclipse.jetty.websocket.api.WebSocketAdapter;
+import org.eclipse.jetty.websocket.client.WebSocketClient;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.net.URI;
+import java.util.concurrent.Future;
+
+
+public class EnvironmentWebsocketTest {
+
+ @BeforeClass
+ public static void init() throws Exception {
+ AbstractSubmarineServerTest.startUp(
+ EnvironmentWebsocketTest.class.getSimpleName());
+ }
+
+ @AfterClass
+ public static void destroy() throws Exception {
+ AbstractSubmarineServerTest.shutDown();
+ }
+
+ @Test
+ public void testWebsocketConnection() throws Exception{
+ URI uri = URI.create(
+ AbstractSubmarineServerTest.getWebsocketApiUrlToTest("environment"));
+ WebSocketClient client = new WebSocketClient();
+
+ try {
+ client.start();
+ // The socket that receives events
+ EventSocket socket = new EventSocket();
+ // Attempt Connect
+ Future<Session> fut = client.connect(socket, uri);
+ // Wait for Connect
+ Session session = fut.get();
+ // Send a message
+ session.getRemote().sendString("Hello");
+ // Close session
+ //session.close();
+ session.close(StatusCode.NORMAL, "I'm done");
+ } finally {
+ client.stop();
+ }
+ }
+
+ public class EventSocket extends WebSocketAdapter
+ {
+ @Override
+ public void onWebSocketConnect(Session sess)
+ {
+ super.onWebSocketConnect(sess);
+ System.out.println("Socket Connected: " + sess);
+ }
+
+ @Override
+ public void onWebSocketText(String message)
+ {
+ super.onWebSocketText(message);
+ System.out.println("Received TEXT message: " + message);
+ }
+
+ @Override
+ public void onWebSocketClose(int statusCode, String reason)
+ {
+ super.onWebSocketClose(statusCode, reason);
+ System.out.println("Socket Closed: [" + statusCode + "] " + reason);
+ }
Review Comment:
Use logging instead of printing the message.
Also, check if the message is correct?
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/websocket/DateJsonDeserializer.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.submarine.server.websocket;
+
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonParseException;
+
+import java.lang.reflect.Type;
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.Locale;
+
+public class DateJsonDeserializer implements JsonDeserializer{
+ private final String[] DATE_FORMATS = new String[] {
+ "yyyy-MM-dd'T'HH:mm:ssZ",
+ "MMM d, yyyy h:mm:ss a",
+ "MMM dd, yyyy HH:mm:ss",
+ "yyyy-MM-dd HH:mm:ss.SSS"
+ };
+
+ @Override
+ public Date deserialize(JsonElement jsonElement, Type typeOF,
+ JsonDeserializationContext context) throws JsonParseException {
+ for (String format : DATE_FORMATS) {
+ try {
+ return new SimpleDateFormat(format,
Locale.US).parse(jsonElement.getAsString());
+ } catch (ParseException e) {
+ // do nothing
Review Comment:
Please throw the error
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/websocket/WebSocketServer.java:
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.submarine.server.websocket;
+
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import java.io.IOException;
+import java.util.Date;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.commons.lang.StringUtils;
+import org.eclipse.jetty.util.annotation.ManagedAttribute;
+import org.eclipse.jetty.util.annotation.ManagedObject;
+import org.eclipse.jetty.util.annotation.ManagedOperation;
+import org.eclipse.jetty.websocket.servlet.WebSocketServlet;
+import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Submarine websocket service. This class used setter injection because all
servlet should have
+ * no-parameter constructor
+ */
+@ManagedObject
+public class WebSocketServer extends WebSocketServlet
+ implements org.apache.submarine.server.websocket.WebSocketListener {
+
+ /**
+ * Job manager service type.
+ */
+ protected enum JobManagerServiceType {
+ JOB_MANAGER_PAGE("JOB_MANAGER_PAGE");
Review Comment:
Why do we need this setting?
##########
submarine-server/server-core/src/test/java/org/apache/submarine/server/AbstractSubmarineServerTest.java:
##########
@@ -75,7 +75,10 @@ public abstract class AbstractSubmarineServerTest {
protected static final Logger LOG =
LoggerFactory.getLogger(AbstractSubmarineServerTest.class);
- static final String WEBSOCKET_API_URL = "/ws";
+ static final String WEBSOCKET_API_URL = "/wss";
+ static final String WEBSOCKET_NOTEBOOK_API_URL = "/ws/notebook";
+ static final String WEBSOCKET_EXPERIMENT_API_URL = "/ws/experiment";
+ static final String WEBSOCKET_ENVIRONMENT_API_URL = "/ws/environment";
Review Comment:
Replace /ws/... with /wss/...?
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/workbench/websocket/NotebookSocket.java:
##########
@@ -29,13 +29,13 @@
*/
public class NotebookSocket extends WebSocketAdapter {
private Session connection;
- private NotebookSocketListener listener;
+ private
org.apache.submarine.server.workbench.websocket.NotebookSocketListener listener;
private HttpServletRequest request;
private String protocol;
private String user;
public NotebookSocket(HttpServletRequest req, String protocol,
- NotebookSocketListener listener) {
+ org.apache.submarine.server.workbench.websocket.NotebookSocketListener
listener) {
Review Comment:
Same as above.
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/workbench/websocket/NotebookServer.java:
##########
@@ -39,7 +39,7 @@
*/
@ManagedObject
public class NotebookServer extends WebSocketServlet
- implements NotebookSocketListener {
+ implements
org.apache.submarine.server.workbench.websocket.NotebookSocketListener {
Review Comment:
No need to change this line.
##########
submarine-server/server-core/src/test/java/org/apache/submarine/server/websocket/ExperimentWebsocketTest.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.submarine.server.websocket;
+
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.StatusCode;
+import org.eclipse.jetty.websocket.api.WebSocketAdapter;
+import org.eclipse.jetty.websocket.client.WebSocketClient;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import java.net.URI;
+import java.util.concurrent.Future;
+import org.apache.submarine.server.AbstractSubmarineServerTest;
+
+
+public class ExperimentWebsocketTest {
+
+ @BeforeClass
+ public static void init() throws Exception {
+ AbstractSubmarineServerTest.startUp(
+ ExperimentWebsocketTest.class.getSimpleName());
+ }
+
+ @AfterClass
+ public static void destroy() throws Exception {
+ AbstractSubmarineServerTest.shutDown();
+ }
+
+ @Test
+ public void testWebsocketConnection() throws Exception{
+ URI uri = URI.create(
+ AbstractSubmarineServerTest.getWebsocketApiUrlToTest("experiment"));
+ WebSocketClient client = new WebSocketClient();
+
+ try {
+ client.start();
+ // The socket that receives events
+ EventSocket socket = new EventSocket();
+ // Attempt Connect
+ Future<Session> fut = client.connect(socket, uri);
+ // Wait for Connect
+ Session session = fut.get();
+ // Send a message
+ session.getRemote().sendString("Hello");
+ // Close session
+ //session.close();
+ session.close(StatusCode.NORMAL, "I'm done");
+ } finally {
+ client.stop();
+ }
+ }
+
+ public class EventSocket extends WebSocketAdapter
+ {
+ @Override
+ public void onWebSocketConnect(Session sess)
+ {
+ super.onWebSocketConnect(sess);
+ System.out.println("Socket Connected: " + sess);
+ }
+
+ @Override
+ public void onWebSocketText(String message)
+ {
+ super.onWebSocketText(message);
+ System.out.println("Received TEXT message: " + message);
+ }
+
+ @Override
+ public void onWebSocketClose(int statusCode, String reason)
+ {
+ super.onWebSocketClose(statusCode, reason);
+ System.out.println("Socket Closed: [" + statusCode + "] " + reason);
+ }
Review Comment:
Same as above
##########
submarine-server/server-core/src/test/java/org/apache/submarine/server/websocket/NotebookWebsocketTest.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.submarine.server.websocket;
+
+import org.apache.submarine.server.AbstractSubmarineServerTest;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.StatusCode;
+import org.eclipse.jetty.websocket.api.WebSocketAdapter;
+import org.eclipse.jetty.websocket.client.WebSocketClient;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.net.URI;
+import java.util.concurrent.Future;
+
+
+public class NotebookWebsocketTest {
+
+ @BeforeClass
+ public static void init() throws Exception {
+ AbstractSubmarineServerTest.startUp(
+ NotebookWebsocketTest.class.getSimpleName());
+ }
+
+ @AfterClass
+ public static void destroy() throws Exception {
+ AbstractSubmarineServerTest.shutDown();
+ }
+
+ @Test
+ public void testWebsocketConnection() throws Exception{
+ URI uri = URI.create(
+ AbstractSubmarineServerTest.getWebsocketApiUrlToTest("notebook"));
+ WebSocketClient client = new WebSocketClient();
+
+ try {
+ client.start();
+ // The socket that receives events
+ EventSocket socket = new EventSocket();
+ // Attempt Connect
+ Future<Session> fut = client.connect(socket, uri);
+ // Wait for Connect
+ Session session = fut.get();
+ // Send a message
+ session.getRemote().sendString("Hello");
+ // Close session
+ //session.close();
+ session.close(StatusCode.NORMAL, "I'm done");
+ } finally {
+ client.stop();
+ }
+ }
+
+ public class EventSocket extends WebSocketAdapter
+ {
+ @Override
+ public void onWebSocketConnect(Session sess)
+ {
+ super.onWebSocketConnect(sess);
+ System.out.println("Socket Connected: " + sess);
+ }
+
+ @Override
+ public void onWebSocketText(String message)
+ {
+ super.onWebSocketText(message);
+ System.out.println("Received TEXT message: " + message);
+ }
+
+ @Override
+ public void onWebSocketClose(int statusCode, String reason)
+ {
+ super.onWebSocketClose(statusCode, reason);
+ System.out.println("Socket Closed: [" + statusCode + "] " + reason);
+ }
Review Comment:
Same as above
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/workbench/websocket/NotebookSocket.java:
##########
@@ -29,13 +29,13 @@
*/
public class NotebookSocket extends WebSocketAdapter {
private Session connection;
- private NotebookSocketListener listener;
+ private
org.apache.submarine.server.workbench.websocket.NotebookSocketListener listener;
Review Comment:
Same as above.
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/websocket/WebSocket.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.submarine.server.websocket;
+
+import org.apache.commons.lang.StringUtils;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.WebSocketAdapter;
+
+import java.io.IOException;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Notebook websocket.
+ */
+public class WebSocket extends WebSocketAdapter {
Review Comment:
1. I figure that the class is a general WebSocket handler not only for
Notebook WebSocket?
2. Is the name "WebSocketHandler" better?
##########
submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmarineServer.java:
##########
@@ -248,14 +254,37 @@ private static Server
setupJettyServer(SubmarineConfiguration conf) {
}
private static void setupNotebookServer(WebAppContext webapp,
- SubmarineConfiguration conf, ServiceLocator serviceLocator) {
+ SubmarineConfiguration conf,
ServiceLocator serviceLocator) {
String maxTextMessageSize = conf.getWebsocketMaxTextMessageSize();
final ServletHolder servletHolder =
new ServletHolder(serviceLocator.getService(NotebookServer.class));
servletHolder.setInitParameter("maxTextMessageSize", maxTextMessageSize);
final ServletContextHandler context = new
ServletContextHandler(ServletContextHandler.SESSIONS);
- webapp.addServlet(servletHolder, "/ws/*");
+ webapp.addServlet(servletHolder, "/wss/*");
+ }
+
+ private static void setupWebSocketServer(WebAppContext webapp,
+ SubmarineConfiguration conf,
ServiceLocator serviceLocator) {
+ String maxTextMessageSize = conf.getWebsocketMaxTextMessageSize();
+ final ServletHolder notebookServletHolder =
+ new ServletHolder(serviceLocator.getService(WebSocketServer.class));
+ notebookServletHolder.setInitParameter("maxTextMessageSize",
maxTextMessageSize);
+
+ final ServletHolder experimentServletHolder =
+ new ServletHolder(serviceLocator.getService(WebSocketServer.class));
+ experimentServletHolder.setInitParameter("maxTextMessageSize",
maxTextMessageSize);
+
+ final ServletHolder environmentServletHolder =
+ new ServletHolder(serviceLocator.getService(WebSocketServer.class));
+ environmentServletHolder.setInitParameter("maxTextMessageSize",
maxTextMessageSize);
+
+
+
+ final ServletContextHandler context = new
ServletContextHandler(ServletContextHandler.SESSIONS);
+ webapp.addServlet(notebookServletHolder, "/ws/notebook/*");
+ webapp.addServlet(experimentServletHolder, "/ws/experiment/*");
+ webapp.addServlet(environmentServletHolder, "/ws/environment/*");
Review Comment:
Replace /ws/... with /wss/... ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
