You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "kezhuw (via GitHub)" <gi...@apache.org> on 2023/09/01 08:12:37 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #2058: ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss
kezhuw opened a new pull request, #2058:
URL: https://github.com/apache/zookeeper/pull/2058
ZooKeeper session will expire approximately after negotiated session timeout. Currently, client will learn this after successful contact to ZooKeeper cluster. This exposes an endless client side connection loss when client can't reach ZooKeeper cluster due to either incomplete connection string or whole cluster downtime.
This pr proposes to introduce a client side session expiration timeout to deal with this. The client side timeout will be approximately `4/3` of server side counterpart, so it can tolerate small skew.
## History
At least four jira issues reported the behavior.
* ZOOKEEPER-2188: client connection hung up because of dead loop
* ZOOKEEPER-4412: client blocked too long before session timeout
* ZOOKEEPER-4508: ZooKeeper client run to endless loop in ClientCnxn.SendThread.run if all server down
* ZOOKEEPER-4692: Handle SessionTimeoutException in Java client
This pr superceded #1847. I did not aware of the fact that ZooKeeper does not have a client side expiration timeout at that time. I will send a proposal email to dev mailing list for discussion soon.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769478455
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1143,7 +1152,7 @@ public void run() {
startConnect(serverAddress);
// Update now to start the connection timer right after we make a connection attempt
clientCnxnSocket.updateNow();
- clientCnxnSocket.updateLastSendAndHeard();
+ clientCnxnSocket.updateLastSend();
Review Comment:
Update `lastSend` so we can measure connection timeout.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#issuecomment-2369637644
I submitted to **master** and **branch-3.9** branches. Thanks!
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#issuecomment-2362087976
@kezhuw This one looks really cool. Want to see this in 3.9.3?
@eolivelli ping.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769478677
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1181,16 +1190,24 @@ public void run() {
}
to = readTimeout - clientCnxnSocket.getIdleRecv();
} else {
- to = connectTimeout - clientCnxnSocket.getIdleRecv();
+ to = connectTimeout - clientCnxnSocket.getIdleSend();
Review Comment:
The old code is source of confusion. In path of old code, we have to update `lastHeard`.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw closed pull request #2058: ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw closed pull request #2058: ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss
URL: https://github.com/apache/zookeeper/pull/2058
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #2058: ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#issuecomment-1719334929
Mail thread for the proposal: https://lists.apache.org/thread/0hf81c1tdsp6mm130gw1h5xglkx48vv0
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769462212
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1274,7 +1295,6 @@ private void cleanAndNotifyState() {
eventThread.queueEvent(new WatchedEvent(Event.EventType.None, Event.KeeperState.Disconnected, null));
Review Comment:
This is the `KeeperState.Disconnected` I mentioned above after disconnected. We should not fire continuous disconnected event without a `KeeperState.SyncConnected`.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769468005
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1274,7 +1295,6 @@ private void cleanAndNotifyState() {
eventThread.queueEvent(new WatchedEvent(Event.EventType.None, Event.KeeperState.Disconnected, null));
Review Comment:
> We should not fire continuous disconnected event without a `KeeperState.SyncConnected`.
This is not true. Currently, we are firing `KeeperState.Disconnected` after every disconnection even for not established connection.
But it should not affect this 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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769461999
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1184,13 +1192,21 @@ public void run() {
to = connectTimeout - clientCnxnSocket.getIdleRecv();
}
- if (to <= 0) {
+ int expiration = expirationTimeout - clientCnxnSocket.getIdleRecv();
+ if (expiration <= 0) {
String warnInfo = String.format(
"Client session timed out, have not heard from server in %dms for session id 0x%s",
clientCnxnSocket.getIdleRecv(),
Long.toHexString(sessionId));
LOG.warn(warnInfo);
+ changeZkState(States.CLOSED);
Review Comment:
`States.CLOSED` is the terminal state for `KeeperState.Expired`. We do this for server side `KeeperState.Expired`, so does client side `KeeperState.Expired`. This way we won't get extra `KeeperState.Disconnected` event, which fired already in `cleanAndNotifyState`, before `KeeperState.Expired`.
https://github.com/apache/zookeeper/blob/bc9afbf8ef1bc6156643d3d05c87fcf8411e9d8f/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L1359-L1370
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] zhaohaidao commented on pull request #2058: ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss
Posted by "zhaohaidao (via GitHub)" <gi...@apache.org>.
zhaohaidao commented on PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#issuecomment-1726049659
IIUC, for zookeeper, session expiration is a serious problem. Zookeeper's tutorial recommends shutting down directly after session expiration (if I understand it wrong, please correct me in time).
Therefore, is it a bit radical for the client to directly announce the expiration after 4/3 negotiation timeout? According to my understanding, during the period when ZookeeperQuorum has no owner (whether a new owner is not elected, or all of them are temporarily hung up, even if it lasts for a long time), The server will not let the session expire. When ZookeeperQuorum elects a new master, the client can continue to provide services as long as it renews Session.
If the client announces that the session has expired after the 4/3 negotiation timeout, but the server will still hold the session for a period of time after resuming service, will this cause trouble for troubleshooting?
In addition, the clock drift of the client cannot be ignored. Regarding the judgment of the Session validity period, it is easier to deal with it based on the server.
Another point, I am not very clear about your proposal, how to deal with the client session after it expires. If the session is re-created, can we compare the advantages and disadvantages of re-creating the session and renewSession?
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769276442
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1274,7 +1295,6 @@ private void cleanAndNotifyState() {
eventThread.queueEvent(new WatchedEvent(Event.EventType.None, Event.KeeperState.Disconnected, null));
}
clientCnxnSocket.updateNow();
- clientCnxnSocket.updateLastSendAndHeard();
Review Comment:
Why have you removed this?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1184,13 +1192,21 @@ public void run() {
to = connectTimeout - clientCnxnSocket.getIdleRecv();
}
- if (to <= 0) {
+ int expiration = expirationTimeout - clientCnxnSocket.getIdleRecv();
+ if (expiration <= 0) {
String warnInfo = String.format(
"Client session timed out, have not heard from server in %dms for session id 0x%s",
clientCnxnSocket.getIdleRecv(),
Long.toHexString(sessionId));
LOG.warn(warnInfo);
+ changeZkState(States.CLOSED);
Review Comment:
Why have you added this here?
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#issuecomment-2365008506
@anmolnar Thank you for your reviewing! I have replied your comments and updated fixup commits.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058#discussion_r1769476369
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1274,7 +1295,6 @@ private void cleanAndNotifyState() {
eventThread.queueEvent(new WatchedEvent(Event.EventType.None, Event.KeeperState.Disconnected, null));
}
clientCnxnSocket.updateNow();
- clientCnxnSocket.updateLastSendAndHeard();
Review Comment:
Because we are not sending nor receiving anything in disconnection. And, session expiration timeout is calculating base on `lastHeard`. If we are going to update it, we will never hit `SessionTimeoutException`.
Besides, `updateLastSendAndHeard` updates both `lastSend` and `lastHeard`. It should be only called for newly established connection. That is one per connection.
1. NIO: https://github.com/apache/zookeeper/blob/bc9afbf8ef1bc6156643d3d05c87fcf8411e9d8f/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNIO.java#L344-L348
2. Netty: https://github.com/apache/zookeeper/blob/bc9afbf8ef1bc6156643d3d05c87fcf8411e9d8f/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNetty.java#L166-L168
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar merged PR #2058:
URL: https://github.com/apache/zookeeper/pull/2058
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org