You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by zenglinxi0615 <gi...@git.apache.org> on 2015/09/23 10:23:18 UTC
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
GitHub user zenglinxi0615 opened a pull request:
https://github.com/apache/incubator-zeppelin/pull/319
ZEPPELIN-312: use threadpoll to do broadcast in zeppelin NotebookServer
as the description in https://issues.apache.org/jira/browse/ZEPPELIN-312 say, broadcaseAll function will call conn.send(serializeMessage(m)) one by one to send message, and if one conn has some problem say: closed or send message timeout, it will block the other socket to get the message, so some zeppelin web url can't show any note in home page.
In this PR, we use threadpoll to do broadcast in zeppelin NotebookServer, so one bad socket connection will not affect the others.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zenglinxi0615/incubator-zeppelin ZEPPELIN-312
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-zeppelin/pull/319.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #319
----
commit 43ff424a570cda0787b67f7f5e2a618d51d03fba
Author: 曾林西 <ze...@meituan.com>
Date: 2015-09-23T04:11:01Z
ZEPPELIN-312: use threadpoll to do broadcast in zeppelin NotebookServer
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by bzz <gi...@git.apache.org>.
Github user bzz commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-166541998
@zenglinxi0615 if you think it's fine, may be we should close this one now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by zenglinxi0615 <gi...@git.apache.org>.
Github user zenglinxi0615 commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-143938382
@Leemoonsoo
Thanks for your advice!
I think it's hard to make a decision, making broadcast() and broadcastAll() waits for message being sent by each threads before return can guarantee message delivery order to the websocket connection, but it will risk to block the notebook server send messege to new websocket connection because of the synchronized (connectedSockets) when there is a bad websocket connection in list, which will lead a bad influence of user experience.
So the key thing here is that is it necessary to guarantee message delivery order to the websocket connection， and why?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-143965595
For example, let's say zeppelin want to send two messages using broadcast() from single thread. The first message is 1) create notebook and second one is 2) list available notebooks.
If broadcast() called in a short time for these two messages, can not guarantee the order of delivery,
so both 1 -> 2 and 2 -> 1 delivery orders are possible, that results inconsistent result in the front-end.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-142536082
Thanks for PR,
Do you have an easy way for us to reproduce that issue?
That would allow us to review this PR and assert that it fix the problem :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by zenglinxi0615 <gi...@git.apache.org>.
Github user zenglinxi0615 commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-142855003
I see.
Actually I have already merge this PR in our branch, and use zeppelin in production environment for several days, it works well so far, and the problem (can't see any note) seems be solved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by zenglinxi0615 <gi...@git.apache.org>.
Github user zenglinxi0615 commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-142557390
There is a tricky way to simulate the bad connected socket situation:
(1) patch this code into zeppelin (this code is used to simulate the bad connected socket situation, for example 1/5 probability）:
```
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java
index aceea45..a0b3be6 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookSocket.java
@@ -66,6 +66,13 @@ public class NotebookSocket implements WebSocket.OnTextMessage{
}
public void send(String serializeMessage) throws IOException {
+ if (Math.random() > 0.8) {
+ try {
+ Thread.sleep(30000);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
connection.sendMessage(serializeMessage);
}
```
(2)compile the zeppelin code and setup an zeppelin server, for example: localhost:8080
(3)open the url: localhost:8080 in several webpage
(4)refresh the webpages, after about five or six times, you will find that you can't see any note in the home page of zeppelin, and opening new window is not work too.
finally, if you patch my PR, and do the same thing as (1)~(4), you will find that you can't see any note in the home page occasionally because of the thread.sleep, but when you refresh the webpage or open a new window, the problem is gone since a new websocket is created and send message successful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by corneadoug <gi...@git.apache.org>.
Github user corneadoug commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-142817833
@zenglinxi0615 Didn't got time to test it yet
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-143106665
Nice improvement.
I think this change does not guarantee message delivery order to the websocket connection.
How about make `broadcast()` and `broadcastAll()` waits for message being sent by each threads before return?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by zenglinxi0615 <gi...@git.apache.org>.
Github user zenglinxi0615 commented on the pull request:
https://github.com/apache/incubator-zeppelin/pull/319#issuecomment-142789535
@corneadoug
Are there any problem about the PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-zeppelin pull request: ZEPPELIN-312: use threadpoll to d...
Posted by zenglinxi0615 <gi...@git.apache.org>.
Github user zenglinxi0615 closed the pull request at:
https://github.com/apache/incubator-zeppelin/pull/319
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---