Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss #2058

Merged
merged 3 commits into from
Sep 23, 2024
Merged

ZOOKEEPER-4508: Expire session in client side to avoid endless connection loss #2058

merged 3 commits into from
Sep 23, 2024

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Sep 1, 2023

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.

@kezhuw kezhuw mentioned this pull request Sep 1, 2023
Closed
@kezhuw kezhuw closed this Sep 3, 2023
@kezhuw kezhuw reopened this Sep 3, 2023
@kezhuw kezhuw force-pushed the ZOOKEEPER-4508-client-side-session-expiration branch from e315a11 to 51fbf2d Compare September 14, 2023 12:13
@kezhuw
Copy link
Member Author

kezhuw commented Sep 14, 2023

Mail thread for the proposal: https://lists.apache.org/thread/0hf81c1tdsp6mm130gw1h5xglkx48vv0

@kezhuw kezhuw mentioned this pull request Sep 19, 2023
Open
@zhaohaidao
Copy link
Contributor

zhaohaidao commented Sep 19, 2023 •
edited
Loading

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 leader (whether a new leader 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 leader, 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?

@anmolnar
Copy link
Contributor

@kezhuw This one looks really cool. Want to see this in 3.9.3?
@eolivelli ping.

@kezhuw
ZOOKEEPER-4508: Expire session in client side to avoid endless connec…
72ffe96 
…tion loss

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.
@kezhuw kezhuw force-pushed the ZOOKEEPER-4508-client-side-session-expiration branch from 51fbf2d to 72ffe96 Compare September 20, 2024 12:21
anmolnar
anmolnar reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few questions.

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java Show resolved Hide resolved
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java Show resolved Hide resolved
kezhuw
kezhuw commented Sep 21, 2024
View reviewed changes
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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update lastSend so we can measure connection timeout.

kezhuw
kezhuw commented Sep 21, 2024
View reviewed changes
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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code is source of confusion. In path of old code, we have to update lastHeard.

@kezhuw
Copy link
Member Author

kezhuw commented Sep 21, 2024

@anmolnar Thank you for your reviewing! I have replied your comments and updated fixup commits.

anmolnar
anmolnar approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@anmolnar anmolnar merged commit 8900618 into apache:master Sep 23, 2024
14 checks passed
asfgit pushed a commit that referenced this pull request Sep 23, 2024
@kezhuw @anmolnar
ZOOKEEPER-4508: Expire session in client side to avoid endless connec…
7f44e33 
…tion loss

Reviewers: anmolnar
Author: kezhuw
Closes #2058 from kezhuw/ZOOKEEPER-4508-client-side-session-expiration

(cherry picked from commit 8900618)
Signed-off-by: Andor Molnar <[email protected]>
@anmolnar
Copy link
Contributor

I submitted to master and branch-3.9 branches. Thanks!

@kezhuw kezhuw deleted the ZOOKEEPER-4508-client-side-session-expiration branch October 14, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmolnar anmolnar anmolnar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kezhuw @zhaohaidao @anmolnar