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.
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.
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.
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.
I vaguely recall this skips the planner not the parser.
There was a problem hiding this comment.
Makes way more sense!
Picks up upstream fix Graceful shutdown and refactor (postgresml#144)
No description provided.