-
Notifications
You must be signed in to change notification settings - Fork 172
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.
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
Graceful shutdown and refactor #144
Conversation
src/client.rs
Outdated
} | ||
|
||
/// Client entrypoint. | ||
pub async fn client_entrypoint( | ||
mut stream: TcpStream, | ||
client_server_map: ClientServerMap, | ||
shutdown_event_receiver: Receiver<()>, | ||
shutdown: Receiver<()>, | ||
drain: Sender<()>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like drain isn't used at all here and we pass to all these functions just to implicitly drop it at the end. We could instead have an explicit drop at the end of the tokio task which is spawned for client entrypoint. This way we can remove unnecessary parameters in these functions
src/main.rs
Outdated
// This is not what we are waiting for instead, we want the receiver to send an error once all senders are closed which is reached after the shutdown event is received | ||
loop { | ||
match tokio::time::timeout( | ||
_ = shutdown_rx.recv() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a different branch for this if the only thing emitting the event is another branch in the select?
https://github.com/levkk/pgcat/pull/144/files?diff=split&w=0#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR199
* Move error response * Adds tests and removes unused variable * Adds debug log
Awesome thanks @zainkabani ! |
@@ -286,8 +342,7 @@ impl ConnectionPool { | |||
Ok(conn) => conn, | |||
Err(err) => { | |||
error!("Banning instance {:?}, error: {:?}", address, err); | |||
self.ban(&address, address.shard, process_id); | |||
self.stats.client_disconnecting(process_id, address.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always wondered about this, we never really disconnected clients that failed to get connection from the pool. Good thing it is fixed now.
@@ -314,7 +372,7 @@ impl ConnectionPool { | |||
|
|||
match tokio::time::timeout( | |||
tokio::time::Duration::from_millis(healthcheck_timeout), | |||
server.query(";"), | |||
server.query(";"), // Cheap query (query parser not used in PG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely recall this skips the planner not the parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes way more sense!
Picks up upstream fix Graceful shutdown and refactor (postgresml#144)
No description provided.