[FLINK-36974]support overwrite flink config by command line #3823

hiliuxg · 2024-12-28T05:24:10Z

Support overwrite flink config in the command line, for example:

bin/flink-cdc.sh1732864461789.yaml --flink-conf execution.checkpointing.interval=10min --flink-conf rest.bind-port=42689 --flink-conf yarn.application.id=application_1714009558476_3563 --flink-conf execution.target=yarn-session --flink-conf rest.bind-address=10.5.140.140

The example provided is used to submit a job to a specified host's YARN session cluster with specific Flink configurations.

yuxiqian

Thanks for @hiliuxg's nice work! Just left some trivial comments.

yuxiqian · 2025-01-06T06:44:19Z

flink-cdc-cli/src/test/java/org/apache/flink/cdc/cli/CliFrontendTest.java

+        assertThat(configMap.get("execution.target")).isEqualTo("yarn-session");
+        assertThat(configMap.get("rest.bind-port")).isEqualTo("42689");
+        assertThat(configMap.get("yarn.application.id"))
+                .isEqualTo("application_1714009558476_3563");
+        assertThat(configMap.get("rest.bind-address")).isEqualTo("10.1.140.140");


Suggested change

assertThat(configMap.get("execution.target")).isEqualTo("yarn-session");

assertThat(configMap.get("rest.bind-port")).isEqualTo("42689");

assertThat(configMap.get("yarn.application.id"))

.isEqualTo("application_1714009558476_3563");

assertThat(configMap.get("rest.bind-address")).isEqualTo("10.1.140.140");

assertThat(configMap)

.containsEntry("execution.target", "yarn-session")

.containsEntry("rest.bind-port", "42689")

.containsEntry("yarn.application.id", "application_1714009558476_3563")

.containsEntry("rest.bind-address", "10.1.140.140");

yuxiqian · 2025-01-06T06:47:11Z

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontendOptions.java

+    public static final Option FLINK_CONFIG =
+            Option.builder("fc")


SQL Client provides -D to pass extra Flink options dynamically. Maybe we can follow the same naming here?

yuxiqian · 2025-01-06T06:48:37Z

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontend.java

+            Configuration flinkConfig, CommandLine commandLine) {
+        String[] flinkConfigs = commandLine.getOptionValues(FLINK_CONFIG);
+        if (flinkConfigs != null) {
+            LOG.info("Find flink config items: {}", String.join(",", flinkConfigs));


Suggested change

LOG.info("Find flink config items: {}", String.join(",", flinkConfigs));

LOG.info("Dynamic flink config items found: {}", flinkConfigs);

yuxiqian · 2025-01-06T06:50:52Z

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontend.java

+                String key = config.split("=")[0].trim();
+                String value = config.split("=")[1].trim();


We can split just once and verify the format first.

yuxiqian · 2025-01-06T06:52:08Z

flink-cdc-cli/src/test/java/org/apache/flink/cdc/cli/CliFrontendTest.java

@@ -145,6 +146,31 @@ void testPipelineExecuting() throws Exception {
        assertThat(executionInfo.getDescription()).isEqualTo("fake-description");
    }

+    @Test
+    void testPipelineExecutingWithFlinkConfig() throws Exception {


Please also add tests for malformed arguments (like when = is missing, or = is part of value, etc.)

hiliuxg · 2025-01-07T04:01:45Z

Hi @yuxiqian, thanks for the review. I have accepted your suggestions and made the necessary modifications. Please review again when you have time. thanks .

flink-cdc-cli/src/test/java/org/apache/flink/cdc/cli/CliFrontendTest.java

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontendOptions.java

hiliuxg · 2025-01-07T07:34:39Z

hi @yuxiqian , thank you for your guidance. I have made another version of the code. Please review it again when you have time.

yuxiqian

Looks nice! Thanks for @hiliuxg's quick response.

Could @leonardBang @ruanhang1993 trigger the CI please?

yuxiqian · 2025-01-07T07:44:31Z

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontend.java

+            String value = properties.getProperty(key);
+            if (StringUtils.isNullOrWhitespaceOnly(key)
+                    || StringUtils.isNullOrWhitespaceOnly(value)) {
+                throw new IllegalArgumentException("Illegal argument for key or value");


nit: maybe we can also print out the questionable key / value pair in command line to improve exception traceability.

done. thanks for comment

yuxiqian · 2025-01-07T09:13:03Z

@hiliuxg Seems there are some test cases in org.apache.flink.cdc.cli.CliFrontendTest that needs to be updated.

yuxiqian · 2025-01-08T02:29:40Z

@hiliuxg org.apache.flink.cdc.cli.CliFrontendTest#testGeneratingHelpMessage and org.apache.flink.cdc.cli.CliFrontendTest#testNoArgument is still failing, presumably because -D <Session dynamic flink config key=val> is so long that output format is changed.

Also, could you please rebase this branch to latest master?

Jzjsnow

I think the HELP_MESSAGE in CliFrontendTest should also be updated to include the -D parameter.

Jzjsnow · 2025-01-09T05:43:19Z

flink-cdc-cli/src/test/java/org/apache/flink/cdc/cli/CliFrontendTest.java

@@ -145,6 +147,70 @@ void testPipelineExecuting() throws Exception {
        assertThat(executionInfo.getDescription()).isEqualTo("fake-description");
    }

+    @Test
+    void testPipelineExecutingWithFlinkConfig() throws Exception {


I'm wondering if we need to add some more cases that test for the concatenation of “-D” and key=value pairs instead of space-separated pairs, such as "-Dexecution.target= yarn-session". I'm concerned that there are cases that might not be recognized, such as "-D=execution.target".

support overwrite flink config by command line

a89259d

github-actions bot added the cli label Dec 28, 2024

hiliuxg changed the title ~~support overwrite flink config by command line~~ [FLINK-36974]support overwrite flink config by command line Dec 28, 2024

yuxiqian reviewed Jan 6, 2025

View reviewed changes

support overwrite flink config by command line

d1764b7

yuxiqian reviewed Jan 7, 2025

View reviewed changes

flink-cdc-cli/src/test/java/org/apache/flink/cdc/cli/CliFrontendTest.java Outdated Show resolved Hide resolved

flink-cdc-cli/src/main/java/org/apache/flink/cdc/cli/CliFrontendOptions.java Outdated Show resolved Hide resolved

support overwrite flink config by command line

b52e798

yuxiqian approved these changes Jan 7, 2025

View reviewed changes

github-actions bot added the reviewed label Jan 7, 2025

format code

6d077f0

Jzjsnow reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-36974]support overwrite flink config by command line #3823

[FLINK-36974]support overwrite flink config by command line #3823

hiliuxg commented Dec 28, 2024

yuxiqian left a comment

yuxiqian Jan 6, 2025

yuxiqian Jan 6, 2025

yuxiqian Jan 6, 2025

yuxiqian Jan 6, 2025

yuxiqian Jan 6, 2025

hiliuxg commented Jan 7, 2025

hiliuxg commented Jan 7, 2025

yuxiqian left a comment •

edited

Loading

yuxiqian Jan 7, 2025 •

edited

Loading

hiliuxg Jan 7, 2025

yuxiqian commented Jan 7, 2025

yuxiqian commented Jan 8, 2025 •

edited

Loading

Jzjsnow left a comment •

edited

Loading

Jzjsnow Jan 9, 2025 •

edited

Loading

		public static final Option FLINK_CONFIG =
		Option.builder("fc")

	LOG.info("Find flink config items: {}", String.join(",", flinkConfigs));
	LOG.info("Dynamic flink config items found: {}", flinkConfigs);

		String key = config.split("=")[0].trim();
		String value = config.split("=")[1].trim();

[FLINK-36974]support overwrite flink config by command line #3823

Are you sure you want to change the base?

[FLINK-36974]support overwrite flink config by command line #3823

Conversation

hiliuxg commented Dec 28, 2024

yuxiqian left a comment

Choose a reason for hiding this comment

yuxiqian Jan 6, 2025

Choose a reason for hiding this comment

yuxiqian Jan 6, 2025

Choose a reason for hiding this comment

yuxiqian Jan 6, 2025

Choose a reason for hiding this comment

yuxiqian Jan 6, 2025

Choose a reason for hiding this comment

yuxiqian Jan 6, 2025

Choose a reason for hiding this comment

hiliuxg commented Jan 7, 2025

hiliuxg commented Jan 7, 2025

yuxiqian left a comment • edited Loading

Choose a reason for hiding this comment

yuxiqian Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

hiliuxg Jan 7, 2025

Choose a reason for hiding this comment

yuxiqian commented Jan 7, 2025

yuxiqian commented Jan 8, 2025 • edited Loading

Jzjsnow left a comment • edited Loading

Choose a reason for hiding this comment

Jzjsnow Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

yuxiqian left a comment •

edited

Loading

yuxiqian Jan 7, 2025 •

edited

Loading

yuxiqian commented Jan 8, 2025 •

edited

Loading

Jzjsnow left a comment •

edited

Loading

Jzjsnow Jan 9, 2025 •

edited

Loading