Adds SHUTDOWN command as alternate option to sending SIGINT #331
Conversation
src/admin.rs
Outdated
| @@ -67,6 +72,11 @@ where | |||
| trace!("RESUME"); | |||
| resume(stream, query_parts[1]).await | |||
| } | |||
| "SHUTDOWN" => { | |||
| trace!("SHUTDOWN"); | |||
| is_shutdown = true; | |||
There was a problem hiding this comment.
Why don't you just send TERM signal to itself here?
let pid = std::process::id;
signal::kill(pid, Signal::TERM);There was a problem hiding this comment.
That's a good idea, I originally thought doing this would pick up the PID of the worker thread but it seems to get the main thread PID.
I've updated the implementation to use that instead, thanks!
|
|
||
| res.put(data_row(&vec![shutdown_success.to_string()])); | ||
|
|
||
| res.put(command_complete("SHUTDOWN")); |
There was a problem hiding this comment.
Typically this would return a SELECT 1, otherwise clients could get confused? Not entirely sure. If you're returning rows, you want to double check that you're using the right protocol syntax.
There was a problem hiding this comment.
Currently all of our other custom protocol commands which return rows follow this pattern, I can change this to a select 1 but it would be different from the rest of our commands
There was a problem hiding this comment.
You're absolutely right. Disregard my comment.
| } | ||
| }; |
There was a problem hiding this comment.
Did you fix indentation here or something else as well? I can't tell from the diff.
There was a problem hiding this comment.
This was all just formatting, it was indented incorrectly
tests/python/tests.py
Outdated
| cur.execute("SELECT 1;") | ||
|
|
||
| # Send SHUTDOWN command pgcat while still in transaction | ||
| admin_cur.execute("SHUTDOWN") |
There was a problem hiding this comment.
This is where you want to check if your protocol implementation works by reading the result.
zainkabani
changed the title
A recent change to ECS has made it so we can no longer send signals to our container during ECS draining phase, an alternate option suggested by @dat2 was to add a SHUTDOWN command as an alternative to initiate the drain process.
This PR adds this admin command, which sends a SIGINT to the process