Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature][Engine] Unify job env parameters #6003

Merged
merged 30 commits into from
Dec 22, 2023

Conversation

happyboy1024
Copy link
Contributor

@happyboy1024 happyboy1024 commented Dec 13, 2023

Purpose of this pull request

  1. Adjust the flink parameter configuration mode by prefixing flink. and the parameter items provided by flink official
  2. Clear outdated configurations in the md file and e2e
    close [Feature][Engine] Unify job env parameters #5937

Does this PR introduce any user-facing change?

How was this patch tested?

add new test cases to separately verify that the parameters are consistent before and after adjustments

Check list

@hailin0
Copy link
Member

hailin0 commented Dec 14, 2023

Please check ci

@hailin0 hailin0 added this to the 2.3.4 milestone Dec 17, 2023
Comment on lines +136 to 153
public static void initTableEnvironmentConfiguration(
Config config, Configuration configuration) {
/**
* flink table configuration items are prefixed with 'table.exec'. reference: {@link
* org.apache.flink.table.api.config.ExecutionConfigOptions}
*/
String prefixConf = "flink.table.exec";
String replacePrefix = "flink.";
if (!config.isEmpty()) {
for (Map.Entry<String, ConfigValue> entryConfKey : config.entrySet()) {
String confKey = entryConfKey.getKey().trim();
if (confKey.startsWith(prefixConf)) {
configuration.setString(
confKey.replaceFirst(prefixConf, ""), entryConfKey.getValue().render());
confKey.replaceFirst(replacePrefix, ""),
entryConfKey.getValue().unwrapped().toString());
}
}
}
Copy link
Member

@Hisoka-X Hisoka-X Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test case for this.

ignore below
Look like it remove prefix of flink.table.exec.config to table.exec.config. Not worked on flink.pipeline.max-parallelism according doc https://github.com/apache/seatunnel/pull/6003/files#diff-8f9b660879c6a25ff9d0f64619025ea7be9a72cd0152c4b896843424cab6abbcR44

Comment on lines +136 to 153
public static void initTableEnvironmentConfiguration(
Config config, Configuration configuration) {
/**
* flink table configuration items are prefixed with 'table.exec'. reference: {@link
* org.apache.flink.table.api.config.ExecutionConfigOptions}
*/
String prefixConf = "flink.table.exec";
String replacePrefix = "flink.";
if (!config.isEmpty()) {
for (Map.Entry<String, ConfigValue> entryConfKey : config.entrySet()) {
String confKey = entryConfKey.getKey().trim();
if (confKey.startsWith(prefixConf)) {
configuration.setString(
confKey.replaceFirst(prefixConf, ""), entryConfKey.getValue().render());
confKey.replaceFirst(replacePrefix, ""),
entryConfKey.getValue().unwrapped().toString());
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but please add a test case for flink.table.exec. feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but please add a test case for flink.table.exec. feature.

I've added a test case for this that uses table env parallelism to override global parallelism. PTAL.

@Slf4j
@DisabledOnContainer(
value = {},
type = {EngineType.SEATUNNEL, EngineType.SPARK},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think we should create new place to store test only for flink in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think we should create new place to store test only for flink in the future.

Agree with you, we can add a new module under seatunnel-engine-e2e, this module can be called seatunnel-engine-flink-e2e or seatunnel-flink-engine-e2e.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Would you mind to do this after this PR be merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Would you mind to do this after this PR be merged?

Of course not, after this pr merge, I will do this.

@hailin0 hailin0 merged commit 2410ab3 into apache:dev Dec 22, 2023
11 checks passed
alextinng pushed a commit to alextinng/seatunnel that referenced this pull request Dec 26, 2023
@happyboy1024 happyboy1024 deleted the unify-parameter branch March 12, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Engine] Unify job env parameters
3 participants