feat(frontend): make KILL process_id work across all worker nodes #21998
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the implementation of the KILL command to support cancelling queries across all worker nodes by using a string representation of the worker process ID. Key changes include:
- Updating the SQL parser and AST definition to accept a worker process ID string instead of a numeric process ID.
- Introducing a new WorkerProcessId struct with proper string conversion and display implementations.
- Extending RPC, session, and worker node management logic to support distributed cancellation via the updated KILL command.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/sqlparser/src/parser.rs | Updated KILL command parsing to handle a literal string. |
| src/sqlparser/src/ast/mod.rs | Changed the KILL variant to accept a String in the AST. |
| src/rpc_client/src/frontend_client.rs | Added a new cancel_running_sql RPC call. |
| src/frontend/src/test_utils.rs | Added a stub worker_id method for tests. |
| src/frontend/src/session.rs | Introduced WorkerProcessId and refactored cancellation helpers. |
| src/frontend/src/rpc/mod.rs | Added cancellation RPC endpoint implementation. |
| src/frontend/src/meta_client.rs | Extended the meta client interface with a worker_id method. |
| src/frontend/src/handler/show.rs | Updated process id formatting to use WorkerProcessId. |
| src/frontend/src/handler/mod.rs | Modified KILL statement handling to pass WorkerProcessId. |
| src/frontend/src/handler/kill_process.rs | Updated kill processing to parse and delegate based on worker process ID. |
| src/frontend/src/error.rs | Adjusted error handling to include relevant conversions. |
| src/batch/src/worker_manager/worker_node_manager.rs | Refactored worker node management using a HashMap instead of Vec. |
| proto/frontend_service.proto | Added CancelRunningSql RPC and message definitions. |
Comments suppressed due to low confidence (1)
src/frontend/src/handler/kill_process.rs:25
- [nitpick] The parameter 's' is ambiguous; consider renaming it to 'worker_process_id_str' to improve code clarity.
pub(super) async fn handle_kill(handler_args: HandlerArgs, s: String) -> Result<RwPgResponse> {
|
Another frontend stack overflow: The total stack size is 2093680 bytes after this PR, which slightly exceeds the 2MB limit. I'll increase the RUST_MIN_STACK to 4MB for tests using the debug build. By the way, I attempted to modify these in |
| } | ||
| } | ||
|
|
||
| impl TryFrom<String> for WorkerProcessId { |
There was a problem hiding this comment.
We may implement FromStr instead.
| @@ -5011,8 +5011,8 @@ impl Parser<'_> { | |||
| } | |||
|
|
|||
| pub fn parse_kill_process(&mut self) -> ModalResult<Statement> { | |||
| let process_id = self.parse_literal_uint()? as i32; | |||
| Ok(Statement::Kill(process_id)) | |||
| let worker_process_id = self.parse_literal_string()?; | |||
There was a problem hiding this comment.
Will this break Postgres compatibility?
|
|
||
| pub(super) async fn handle_kill( | ||
| handler_args: HandlerArgs, | ||
| pub(super) async fn handle_kill(handler_args: HandlerArgs, s: String) -> Result<RwPgResponse> { |
There was a problem hiding this comment.
Do we handle permission for KILL?
| if [[ -z "${RUST_MIN_STACK}" ]]; then | ||
| export RUST_MIN_STACK=4194304 | ||
| fi |
There was a problem hiding this comment.
I think we'd better find out why the stack usage increases, other than relaxing the constraints. 🤔
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Resolve #21953
Example
Checklist
Documentation
Release note