[flink] Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys #2279

sd4324530 · 2025-12-29T16:16:15Z

Purpose

Linked issue: close #2270

Brief change log

Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys

wuchong · 2026-01-04T14:44:44Z

@platinumhamburg could you help to review this?

wuchong · 2026-01-04T14:45:18Z

Besides, the coverage reports error.

Warning:  Rule violated for class org.apache.fluss.flink.procedure.ResetClusterConfigsProcedure: lines covered ratio is 0.66, but expected minimum is 0.70

Copilot

Pull request overview

This PR refactors Flink procedure APIs to support multiple configuration keys in a single call. The changes rename procedures from singular (sys.set_cluster_config, sys.get_cluster_config) to plural forms (sys.set_cluster_configs, sys.get_cluster_configs) and introduce a new sys.reset_cluster_configs procedure to separate the reset functionality.

Key changes:

Refactored procedures to use varargs, enabling multiple config keys/pairs in single calls
Split delete/reset functionality from set_cluster_config into a dedicated reset_cluster_configs procedure
Updated all procedure names to plural forms for consistency

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
FlinkProcedureITCase.java	Updated test cases to use renamed procedures and added coverage for multi-config operations
SetClusterConfigsProcedure.java	Refactored to accept varargs for config key-value pairs, enabling setting multiple configs at once
ResetClusterConfigsProcedure.java	New procedure dedicated to resetting/deleting multiple cluster configurations
GetClusterConfigsProcedure.java	Enhanced to retrieve multiple specific configurations using varargs
ProcedureManager.java	Updated procedure registrations to reflect renamed and new procedures

Comments suppressed due to low confidence (5)

fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java:32

The documentation mentions that this procedure can "set or delete cluster configuration dynamically", but the delete functionality has been moved to a separate procedure (reset_cluster_configs). This comment should be updated to only mention "set" to reflect the actual functionality.
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java:54
The documentation states that empty string ('') can be used to delete a configuration, but this functionality has been moved to the reset_cluster_configs procedure. This example should be removed or updated to clarify that resetting should be done via the reset_cluster_configs procedure.
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java:51
Missing space after the comma in the example. Should be: 'CALL sys.set_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec', '200MB', 'datalake.format', 'paimon');'
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java:72
The error message should use "configuration pairs" (plural) instead of "configuration pairs." to be consistent with other error messages in the procedure. Consider: "Please specify valid configuration pairs."
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java:78
The error message should use "configuration pairs" (plural) instead of "configuration pairs." to be consistent. Consider: "Please specify valid configuration pairs."

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-04T14:54:46Z

.../fluss-flink-common/src/test/java/org/apache/fluss/flink/procedure/FlinkProcedureITCase.java

+                                        .await())
+                .rootCause()
+                .hasMessageContaining(
+                        "config_pairs must be set in pairs. Please specify a valid configuration pairs.");


The expected error message uses "configuration pairs." with a period before the end quote, but this should be "configuration pairs" without the period for proper grammar. The period should only appear after the closing quote in a sentence.

Suggested change

"config_pairs must be set in pairs. Please specify a valid configuration pairs.");

"config_pairs must be set in pairs. Please specify valid \"configuration pairs\".");

Copilot · 2026-01-04T14:54:46Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+ * CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec');
+ *
+ * -- reset multiple configurations at one time
+ * CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec','datalake.format');


Missing space after the comma in the example. Should be: 'CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec', 'datalake.format');'

Suggested change

* CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec','datalake.format');

* CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec', 'datalake.format');

Copilot · 2026-01-04T14:54:46Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+                if (configKey.isEmpty()) {
+                    throw new IllegalArgumentException(
+                            "Config key cannot be null or empty. "
+                                    + "Please specify valid configuration key.");


The error message should say "a valid configuration key" (singular) instead of "valid configuration key" to be grammatically correct and consistent with similar error messages in the codebase.

Suggested change

+ "Please specify valid configuration key.");

+ "Please specify a valid configuration key.");

Copilot · 2026-01-04T14:54:46Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+            // Validate config key
+            if (configKeys.length == 0) {
+                throw new IllegalArgumentException(
+                        "config_pairs cannot be null or empty. "


The error message mentions "config_pairs" but should mention "config_keys" to match the parameter name used in this procedure. The parameter is named "config_keys" in the @ArgumentHint annotation.

Suggested change

"config_pairs cannot be null or empty. "

"config_keys cannot be null or empty. "

platinumhamburg · 2026-01-05T02:59:37Z

@platinumhamburg could you help to review this?

Sure, I'll review this PR.

sd4324530 · 2026-01-05T07:35:05Z

@platinumhamburg
fix done, please take a look

platinumhamburg

@sd4324530 This PR LGTM overall. I have a few minor suggestions, and additionally, the documentation should be updated accordingly based on the changes in this PR.

platinumhamburg · 2026-01-05T07:26:40Z

.../fluss-flink-common/src/test/java/org/apache/fluss/flink/procedure/FlinkProcedureITCase.java


    @Test
-    void testGetClusterConfig() throws Exception {
+    void testGetClusterConfigs() throws Exception {


Need to enrich this case by covering scenarios, including:

retrieval of multiple configurations

some configurations do not exist.

platinumhamburg · 2026-01-05T07:39:13Z

...-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java

 * -- Delete a configuration (reset to default)
- * CALL sys.set_cluster_config('kv.rocksdb.shared-rate-limiter.bytes-per-sec', NULL);
- * CALL sys.set_cluster_config('kv.rocksdb.shared-rate-limiter.bytes-per-sec', '');
+ * CALL sys.set_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec', '');


Remove, not supported.

platinumhamburg · 2026-01-05T07:47:12Z

...-flink-common/src/main/java/org/apache/fluss/flink/procedure/SetClusterConfigsProcedure.java

-                        "Successfully %s configuration '%s'. "
-                                + "The change is persisted in ZooKeeper and applied to all servers.",
-                        operationDesc, configKey)
+                resultMessage + "The change is persisted in ZooKeeper and applied to all servers."


The current resultMessage aggregates the processing results of all requests into a single line, resulting in poor readability. Since the return value supports returning a String[], please refactor the code to improve readability.

platinumhamburg · 2026-01-05T07:49:18Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+ * <p>This procedure allows modifying dynamic cluster configurations. The changes are:
+ *
+ * <ul>
+ *   <li>Validated by the CoordinatorServer before persistence


The comment description should be updated. In theory, an operation like "Reset to default value" should always succeed, as the default value should be a valid one.

platinumhamburg · 2026-01-05T07:49:24Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+ *
+ * </pre>
+ *
+ * <p><b>Note:</b> Not all configurations support dynamic changes. The server will validate the


platinumhamburg · 2026-01-05T07:57:21Z

...link-common/src/main/java/org/apache/fluss/flink/procedure/ResetClusterConfigsProcedure.java

+            admin.alterClusterConfigs(configList).get();
+
+            return new String[] {
+                resultMessage + "The change is persisted in ZooKeeper and applied to all servers."


Ditto, optimize the returned message—do not aggregate responses from all requests into a single line.

platinumhamburg · 2026-01-05T08:02:44Z

.../fluss-flink-common/src/test/java/org/apache/fluss/flink/procedure/FlinkProcedureITCase.java


    @Test
    void testSetClusterConfigValidation() throws Exception {
        // Try to set an invalid config (not allowed for dynamic change)


Need to enrich the test scenarios covered in this test case, including:

No parameters passed;

Mismatched key-value pairs in the input parameters.

sd4324530 · 2026-01-05T15:32:18Z

@platinumhamburg
I reverted to the first commit and resubmitted, but the SetClusterConfigProcedure file is still being deleted. This is strange, and I haven't resolved this issue yet.

platinumhamburg · 2026-01-06T01:22:50Z

I reverted to the first commit and resubmitted, but the SetClusterConfigProcedure file is still being deleted. This is strange, and I haven't resolved this issue yet.

@sd4324530 No worries—this is actually expected behavior. When you revert to an earlier commit and resubmit, Git (and GitHub) determine whether a file is "renamed" or "deleted + added" based on content similarity. If the new version of SetClusterConfigProcedure differs significantly from the original (i.e., less than 50% of the lines are unchanged), Git treats it as a deletion of the old file and creation of a new one, rather than a rename. So even though you reverted and resubmitted, the system still shows it as deleted because the content similarity fell below the threshold. Overall, this aligns with how Git’s rename detection works.

platinumhamburg · 2026-01-06T01:25:22Z

website/docs/engine-flink/procedures.md

+
+**Important Notes:**
+
+- Changes are validated before being applied and persisted in ZooKeeper


The description here is still a bit odd, because the user isn't trying to set a new value—they're simply reverting to the default configuration, which should always be valid.

platinumhamburg · 2026-01-06T01:26:57Z

website/docs/engine-flink/procedures.md

+
+- Changes are validated before being applied and persisted in ZooKeeper
+- Changes are automatically applied to all servers (Coordinator and TabletServers)
+- Changes survive server restarts


This short description would be better placed in the set_cluster_configs section.

platinumhamburg · 2026-01-06T01:33:30Z

.../fluss-flink-common/src/test/java/org/apache/fluss/flink/procedure/FlinkProcedureITCase.java

+                                        .await())
+                .rootCause()
+                .hasMessageContaining(
+                        "The config key invalid.config.key is not allowed to be changed dynamically");


An invalid (non-existent) key and a key that isn't allowed to be set dynamically are two distinct error cases. The current error message can be misleading. If we prefer not to complicate the validation logic, we could instead use "or" in the error message to cover both scenarios explicitly.

…conifg keys Signed-off-by: Pei Yu <[email protected]>

Signed-off-by: Pei Yu <[email protected]>

platinumhamburg · 2026-01-07T02:26:18Z

fluss-server/src/main/java/org/apache/fluss/server/DynamicConfigManager.java

                                String.format(
-                                        "The config key %s is not allowed to be changed dynamically.",
+                                        "The config key %s is not allowed to be changed dynamically or invalid (non-existent).",
                                        configPropName));


@sd4324530 Sorry, you misunderstood my point. What I meant was that, from a testing perspective, the exposed error message doesn't accurately reflect the underlying issue (and indeed, this wasn't introduced by your PR). I don't recommend simply modifying the error message thrown in DynamicConfigManager.java to meet the requirement, as this wouldn't actually address the root problem—that the validation error messages aren't properly categorized. If you don't intend to fix this issue now, I suggest converting the comment into a TODO note explaining the situation and reverting the changes made in DynamicConfigManager.java.

Additionally, I think the PR is overall quite mature and, after addressing these minor adjustments, it should be ready to move into the final review stage.

@platinumhamburg Okay, I think this issue might require a design adjustment to DynamicConfigManager.alterConfigs.

Signed-off-by: peiyu <[email protected]>

sd4324530 marked this pull request as draft December 29, 2025 16:16

sd4324530 marked this pull request as ready for review December 30, 2025 16:02

sd4324530 marked this pull request as draft December 31, 2025 16:03

sd4324530 changed the title ~~[flink] CALL procedure "sys.set_cluster_config" should support to set multiple conifg options~~ [flink] Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys Dec 31, 2025

sd4324530 force-pushed the issue-2270 branch 2 times, most recently from ff5c87d to 5c54a55 Compare January 4, 2026 12:29

sd4324530 mentioned this pull request Jan 4, 2026

Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys #2270

Open

2 tasks

wuchong requested a review from Copilot January 4, 2026 14:45

Copilot started reviewing on behalf of wuchong January 4, 2026 14:45 View session

Copilot AI reviewed Jan 4, 2026

View reviewed changes

sd4324530 force-pushed the issue-2270 branch from 45d5d60 to 08e8df2 Compare January 5, 2026 02:55

sd4324530 force-pushed the issue-2270 branch from 08e8df2 to 59241ce Compare January 5, 2026 03:13

sd4324530 marked this pull request as ready for review January 5, 2026 03:19

sd4324530 force-pushed the issue-2270 branch from 59241ce to e89db9e Compare January 5, 2026 03:54

platinumhamburg suggested changes Jan 5, 2026

View reviewed changes

sd4324530 force-pushed the issue-2270 branch 2 times, most recently from 216ae0d to 5883c0a Compare January 5, 2026 15:29

platinumhamburg suggested changes Jan 6, 2026

View reviewed changes

sd4324530 added 3 commits January 6, 2026 22:29

Refactor CALL procedure "sys.xxx_cluster_config" to support multiple …

05d9c23

…conifg keys Signed-off-by: Pei Yu <[email protected]>

Refactor CALL procedure "sys.xxx_cluster_config" to support multiple …

0ae691e

…conifg keys Signed-off-by: Pei Yu <[email protected]>

fix comment

053dd5d

Signed-off-by: Pei Yu <[email protected]>

sd4324530 force-pushed the issue-2270 branch 2 times, most recently from f48d7e1 to 55e31bd Compare January 7, 2026 00:11

platinumhamburg suggested changes Jan 7, 2026

View reviewed changes

sd4324530 force-pushed the issue-2270 branch from 55e31bd to 57c7c19 Compare January 7, 2026 07:30

fix comment

c1f8a3c

Signed-off-by: peiyu <[email protected]>

sd4324530 force-pushed the issue-2270 branch from 57c7c19 to c1f8a3c Compare January 7, 2026 07:42

	"config_pairs must be set in pairs. Please specify a valid configuration pairs.");
	"config_pairs must be set in pairs. Please specify valid \"configuration pairs\".");

	* CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec','datalake.format');
	* CALL sys.reset_cluster_configs('kv.rocksdb.shared-rate-limiter.bytes-per-sec', 'datalake.format');

	+ "Please specify valid configuration key.");
	+ "Please specify a valid configuration key.");

	"config_pairs cannot be null or empty. "
	"config_keys cannot be null or empty. "


		Important Notes:

		- Changes are validated before being applied and persisted in ZooKeeper

[flink] Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys #2279

Are you sure you want to change the base?

[flink] Refactor CALL procedure "sys.xxx_cluster_config" to support multiple conifg keys #2279

Conversation

sd4324530 commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Brief change log

Uh oh!

wuchong commented Jan 4, 2026

Uh oh!

wuchong commented Jan 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

platinumhamburg commented Jan 5, 2026

Uh oh!

sd4324530 commented Jan 5, 2026

Uh oh!

platinumhamburg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sd4324530 commented Jan 5, 2026

Uh oh!

platinumhamburg commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sd4324530 commented Dec 29, 2025 •

edited

Loading