You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "johannesschillinger-dm (via GitHub)" <gi...@apache.org> on 2024/08/23 10:41:22 UTC
[PR] [ZEPPELIN-1480] synchronize websocket sync to prevent issues with blocked websockets due to multithreading [zeppelin]
johannesschillinger-dm opened a new pull request, #4800:
URL: https://github.com/apache/zeppelin/pull/4800
### What is this PR for?
Zeppelin Frontend sometimes stops working with following error in the log:
"Blocking message pending 10000 for BLOCKING"
According to Jetty documentation, Websocket writes need to be thread safe or the async functions need to be used.
This PR addresses the issue and makes the send requests thread safe
### What type of PR is it?
Bug Fix
### Todos
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1480
### How should this be tested?
This problem only occurs in production size deployments and is inherently a race condition
### Questions:
* Does the license files need to 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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] synchronize websocket sync to prevent issues with blocked websockets due to multithreading [zeppelin]
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#discussion_r1730203344
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -42,7 +42,7 @@ public String getHeader(String key) {
return String.valueOf(headers.get(key));
}
- public void send(String serializeMessage) throws IOException {
+ public synchronized void send(String serializeMessage) throws IOException {
Review Comment:
synchronized method is not a proper solution for it. How about making it an asynchronous function?
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#discussion_r1750458212
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,26 +24,35 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ String headerValue = String.valueOf(headers.get(key));
+ LOGGER.debug("Getting header: {} with value: {}", key, headerValue);
+ return headerValue;
}
public void send(String serializeMessage) throws IOException {
- session.getBasicRemote().sendText(serializeMessage);
+ session.getAsyncRemote().sendText(serializeMessage, result -> {
+ if (result.getException() != null) {
+ LOGGER.error("Failed to send async message", result.getException());
+ }
+ });
}
public String getUser() {
+ LOGGER.debug("Getting user: {}", user);
return user;
}
public void setUser(String user) {
+ LOGGER.debug("Setting user: {}", user);
this.user = user;
}
@Override
public String toString() {
return ServerUtils.getRemoteAddress(session);
}
-}
+}
Review Comment:
Please make sure to insert a newline at the end.
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,26 +24,35 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ String headerValue = String.valueOf(headers.get(key));
+ LOGGER.debug("Getting header: {} with value: {}", key, headerValue);
+ return headerValue;
}
public void send(String serializeMessage) throws IOException {
- session.getBasicRemote().sendText(serializeMessage);
+ session.getAsyncRemote().sendText(serializeMessage, result -> {
Review Comment:
This solution looks much better than the synchronize block
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,26 +24,35 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ String headerValue = String.valueOf(headers.get(key));
+ LOGGER.debug("Getting header: {} with value: {}", key, headerValue);
+ return headerValue;
}
public void send(String serializeMessage) throws IOException {
- session.getBasicRemote().sendText(serializeMessage);
+ session.getAsyncRemote().sendText(serializeMessage, result -> {
+ if (result.getException() != null) {
+ LOGGER.error("Failed to send async message", result.getException());
Review Comment:
The user and the session may be helpful here.
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -1,23 +1,9 @@
-/*
Review Comment:
Please do not remove the license text.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer merged PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "johannesschillinger-dm (via GitHub)" <gi...@apache.org>.
johannesschillinger-dm commented on code in PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#discussion_r1753153972
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,26 +24,35 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ String headerValue = String.valueOf(headers.get(key));
+ LOGGER.debug("Getting header: {} with value: {}", key, headerValue);
+ return headerValue;
}
public void send(String serializeMessage) throws IOException {
- session.getBasicRemote().sendText(serializeMessage);
+ session.getAsyncRemote().sendText(serializeMessage, result -> {
+ if (result.getException() != null) {
+ LOGGER.error("Failed to send async message", result.getException());
Review Comment:
good idea - on 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.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "johannesschillinger-dm (via GitHub)" <gi...@apache.org>.
johannesschillinger-dm commented on code in PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#discussion_r1762515956
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,21 +40,27 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ return String.valueOf(headers.get(key));
Review Comment:
done
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#discussion_r1754319428
##########
zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java:
##########
@@ -36,21 +40,27 @@ public NotebookSocket(Session session, Map<String, Object> headers) {
this.session = session;
this.headers = headers;
this.user = StringUtils.EMPTY;
+ LOGGER.debug("NotebookSocket created for session: {}", session.getId());
}
public String getHeader(String key) {
- return String.valueOf(headers.get(key));
+ return String.valueOf(headers.get(key));
Review Comment:
nit - please remove the two spaces. Zeppelin uses in most java classes an indentation of two spaces
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#issuecomment-2355377762
Thank you very much for your contribution. Hoping for more.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#issuecomment-2355080710
Nope. LGTM
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-1480] rework websocket sending to prevent partial frontend hangup [zeppelin]
Posted by "johannesschillinger-dm (via GitHub)" <gi...@apache.org>.
johannesschillinger-dm commented on PR #4800:
URL: https://github.com/apache/zeppelin/pull/4800#issuecomment-2337758522
@jongyoul thank you for your feedback - I reworked the PR.
Our production system suffered heavily from frontend lockups due to this issue. With the proposed patch applied, Zeppelin remains stable.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org