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

[FLINK-35700][cli] Loosen CDC pipeline options validation #3435

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

yuxiqian
Copy link
Contributor

@yuxiqian yuxiqian commented Jun 26, 2024

This closes FLINK-35700.

FLINK-35121 adds pipeline configuration validation, rejecting any unknown options, which turns out to be too strict, and it's not possible to create customized configuration extensions. Also, Flink doesn't reject unknown entries in flink-conf / config.yaml, just silently ignores them. It might be better for CDC to follow such behavior.

@yuxiqian
Copy link
Contributor Author

@leonardBang @loserwang1024 PTAL

String.format("Unknown configuration key `%s`", key));
}
});

options.forEach(
option -> {
if (!configuration.getOptional(option).isPresent()
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I also don't understand why unspecified configuration without default value is error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Flink config also have a sink.parallelism, but also no default value.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since required / optional configuration in pipeline is vague (and undocumented), I've removed all pipeline-level validations.

@leonardBang
Copy link
Contributor

@leonardBang @loserwang1024 PTAL

+1 to follow Flink Conf‘s validation style

@loserwang1024
Copy link
Contributor

LGTM!

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

+1

@leonardBang leonardBang merged commit 0723009 into apache:master Jun 26, 2024
15 checks passed
wuzhenhua01 pushed a commit to wuzhenhua01/flink-cdc-connectors that referenced this pull request Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants