feat: Add support for altering source connector properties #20780
Conversation
This commit introduces the ability to alter connector properties for sources using the `ALTER SOURCE ... CONNECTOR WITH` syntax. It includes: - Adding a new handler for altering source connector properties - Updating the SQL parser to support the new syntax - Implementing validation checks to prevent modifying connection-defined properties
- Added `alter_source_connector_props` method to `FrontendMetaClient` and its implementation in `FrontendMetaClientImpl`. - Introduced a mock implementation in `MockFrontendMetaClient`. - Created a new handler `handle_alter_source_connector_props` to manage altering source connector properties. - Removed the obsolete `alter_connector_props.rs` file and updated references to the new handler. - Enhanced `resolve_connection_ref_and_secret_ref` utility to support the new functionality.
…mprove error handling for source connectors
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for altering source connector properties using the new syntax "ALTER SOURCE ... CONNECTOR WITH (...)" and updates the related parsing, AST representation, metadata handling, RPC services, and front-end handlers.
- Added a new parsing branch and AST variant for ALTER CONNECTOR properties in the SQL parser.
- Expanded the meta client, controller, and streaming job management to support updating connector properties for sources.
- Updated front-end handlers and utility functions to integrate with the new ALTER SOURCE CONNECTOR functionality.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/sqlparser/src/parser.rs | Added parsing branch for the CONNECTOR keyword and updated error messages. |
| src/sqlparser/src/ast/ddl.rs | Introduced the new AST variant for AlterConnectorProps and its Display implementation. |
| src/rpc_client/src/meta_client.rs | Added an async function to call alter_source_connector_props with the appropriate parameters. |
| src/meta/src/stream/source_manager.rs | Updated comments for connector parameters to reflect changes for source_id usage. |
| src/meta/src/manager/metadata.rs | Added update_source_props_by_source_id for updating source properties with new connector props. |
| src/meta/src/controller/streaming_job.rs | Extended logic for updating source properties and connector properties in streaming jobs; contains a naming issue. |
| src/meta/service/src/stream_service.rs | Updated service branch to handle both sink and source connector property alterations. |
| src/frontend/src/utils/with_options.rs | Adjusted comments and logic when merging connection properties with provided options. |
| src/frontend/* | Added new handler module and updated meta client and catalog functions to support the ALTER SOURCE CONNECTOR feature. |
| src/connector/* | Enhanced with_options resolution and added a new const to support source alter config list. |
Comments suppressed due to low confidence (1)
src/meta/src/controller/streaming_job.rs:1664
- The variable name 'rewrirte_sql' appears to be misspelled. Consider renaming it to 'rewrite_sql' for clarity.
let rewrirte_sql = {
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for altering source connector properties via a new ALTER SOURCE … CONNECTOR WITH SQL syntax, wiring changes from frontend parsing through RPC, metadata updates, and worker propagation.
- Introduce
alter_source_connector_propsRPC and client methods - Implement metadata update (
update_source_props_by_source_id) and fragment rewriting - Extend source manager and worker to apply updated connector properties at runtime
- Add frontend handler and SQL plumbing for
ALTER SOURCE/SINK … CONNECTOR WITH
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rpc_client/src/meta_client.rs | Exposed alter_source_connector_props on MetaClient |
| src/meta/src/stream/source_manager/worker.rs | Handle UpdateProps command in ConnectorSourceWorker |
| src/meta/src/stream/source_manager.rs | Enqueue UpdateSourceProps in SourceManagerCore |
| src/meta/src/controller/streaming_job.rs | Added update_source_props_by_source_id and SQL rewrite |
| src/meta/service/src/stream_service.rs | Route AlterConnectorPropsRequest for sources and sinks |
| src/frontend/src/handler/alter_source_props.rs | Frontend SQL handler for ALTER SOURCE CONNECTOR |
| src/frontend/src/utils/with_options.rs | Updated doc comments for option resolution |
| src/connector/src/source/mod.rs | Defined allowed alterable props whitelist via LazyLock |
Comments suppressed due to low confidence (2)
src/meta/src/stream/source_manager.rs:114
- Typo in variable name ‘reccreate_source_id_map_new_props’; consider renaming to ‘recreate_source_id_map_new_props’ for clarity.
let mut reccreate_source_id_map_new_props: Vec<(u32, HashMap<String, String>)> =
src/rpc_client/src/meta_client.rs:1326
- The new RPC method
alter_source_connector_propslacks unit or integration tests; consider adding coverage to verify request dispatch and error scenarios.
pub async fn alter_source_connector_props(
| @@ -2086,3 +2336,55 @@ pub struct SinkIntoTableContext { | |||
| /// otherwise empty. | |||
| pub updated_sink_catalogs: Vec<SinkId>, | |||
| } | |||
|
|
|||
| async fn update_connector_props_fragaments<F>( | |||
There was a problem hiding this comment.
Function name ‘update_connector_props_fragaments’ is misspelled; rename to ‘update_connector_props_fragments’.
| async fn update_connector_props_fragaments<F>( | |
| async fn update_connector_props_fragments<F>( |
Copilot uses AI. Check for mistakes.
| @@ -145,6 +145,15 @@ impl CommandContext { | |||
| .unregister_table_ids(unregistered_state_table_ids.iter().cloned()) | |||
| .await?; | |||
| } | |||
| Command::ConnectorPropsChange(obj_id_map_props) => { | |||
There was a problem hiding this comment.
Barrier command handler only applies UpdateSourceProps and ignores sinks or connections; update the command to carry object type and dispatch accordingly.
Copilot uses AI. Check for mistakes.
| @@ -703,6 +705,17 @@ impl MetadataManager { | |||
| .collect()) | |||
| } | |||
|
|
|||
| pub async fn update_source_props_by_source_id( | |||
There was a problem hiding this comment.
Public method lacks a doc comment; please add Rustdoc describing purpose, arguments, and return value of update_source_props_by_source_id.
Copilot uses AI. Check for mistakes.
| @@ -86,6 +87,8 @@ pub trait SourceProperties: | |||
| type SplitEnumerator: SplitEnumerator<Properties = Self, Split = Self::Split>; | |||
| type SplitReader: SplitReader<Split = Self::Split, Properties = Self>; | |||
|
|
|||
| const SOURCE_ALTER_CONFIG_LIST: Set<&'static str> = phf_set! {}; | |||
|
|
|||
There was a problem hiding this comment.
SOURCE_ALTER_CONFIG_LIST is defined but never used; remove or integrate it where connector alterable props are checked.
| /// Checks if a given property is alterable. | |
| fn is_alterable_property(key: &str) -> bool { | |
| Self::SOURCE_ALTER_CONFIG_LIST.contains(key) | |
| } |
Copilot uses AI. Check for mistakes.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This commit introduces the ability to alter connector properties for sources using the
ALTER SOURCE ... CONNECTOR WITHsyntax.related #20691
Checklist
Documentation
Release note
support new syntax
to alter the connector props
Note that the props defined in CONNECTION should be altered by
ALTER CONNECTIONThis is a new feature and we just allow altering props within the whitelist & not change the state table