Implementing request timeout support to ensure when things like network stall the socket or the ZooKeeper server instance freezes close and reconnect is attempted #16
Conversation
…ike network stall the socket or the ZooKeeper server instance freezes
|
@alexguan hey - the lack of request timeout is causing several issues using the library on our team, our callbacks never get called with failure events until a very long time after a connection stalls or process freezes. I have tested this fix extensively with things like stalling the network and freezing the server instance and it seems to be very robust for our use case, thought it would be good to see if you would like to include in the core library. |
…al var declaration at previous var statement
…w option behavior
|
I will take a look at this this weekend. |
|
Thanks mate! |
|
@alexguan any thoughts? |
|
Thanks for the PR, I'm investigating the current Java implementation to see if there is any existing solutions to this timeout issue. |
|
@alexguan any results from your investigation? I had to ship with a private build of this unfortunately and would love to have it in the mainline. I actually updated this PR to default to no requestTimeout (use boolean false) so this PR will not affect existing users out there. I'd love to have a 1:1 conversation some time about your thoughts on this, over IM or voice - its a feature I think that this client really desperately needs for production use. |
|
Sorry Rob, traveling in Japan now, I will try to have a solution this week. |
|
@alexguan hope Japan was fun! Any thoughts/word? |
…connecting under heavy network IO load - should keep trying to reconnect
|
I updated the expiry disconnect behavior just now by the way to retry on session expiry which can happen when reconnecting under heavy network IO load - should keep trying to reconnect. We have had issues in our perf testing where this would occur and the client would be dead in the water and stop trying to reconnect. |
|
@alexguan care to weigh in? |
| @@ -260,7 +278,7 @@ ConnectionManager.prototype.onSocketClosed = function (hasError) { | |||
| break; | |||
| case STATES.SESSION_EXPIRED: | |||
| errorCode = Exception.SESSION_EXPIRED; | |||
| retry = false; | |||
| retry = true; | |||
There was a problem hiding this comment.
The correct way to handle session expire is to create a new session and should handle by the client instead of the library.
Could you explain why you want to retry here?
There was a problem hiding this comment.
Interesting, considering the retry logic I had assumed that the client was intended for indefinite retry/long lived persistent connections.
No problems, can revert this change and then create a new client class if we want to reconnect with a new session?
There was a problem hiding this comment.
My main point is that the session expire should not be handled by the client library but application logic, since only application knows what's the correct action for a session expire event. You don't want to hide it from the client.
|
Sure thing. I'll revert this tonight, just out at dinner atm :) |
|
This is stopping me as well, @alexguan can this PR be merged? Thanks |
|
@alexguan +1 to @mosnicholas since druid-query depends on your module. P.S.: are you still interested and have time for this project? |
|
I'm investigating the current Java client implementation to make sure the connection timeout is handled in the same manner. |
|
So? 😉 @alexguan |
|
+1 |
|
@alexguan |
|
@alexguan no, never? 😛 |
|
It seems like this project is no longer maintained |
|
Please merge the PR |
|
Bump |
No description provided.