-
Notifications
You must be signed in to change notification settings - Fork 490
[flink] Support custom properties in Flink table factory #2431
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
base: main
Are you sure you want to change the base?
Conversation
5f50aa7 to
793d437
Compare
luoyuxia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiebingYu Thanks for thr pr. LGTM @loserwang1024 Could you please help review again?
3c61339 to
a845693
Compare
a845693 to
784312a
Compare
|
@LiebingYu Should we add a specially prefix to custom properties rather than skip all the check? I think it's dangerous for later compability. Currently org.apache.fluss.flink.utils.FlinkConnectorOptionsUtils#validateTableSourceOptions only check the scan start up. why not when catalog read from table, add a prefixes 'customer.' to customers. Then table factory check it and remove the prefixes? @leonardBang @wuchong , CC, WDYT |
+1 for adding a specific prefix for custom properties instead of removing all existing validation for near all options, it make the code being weak. Imaging kafka and MySQL CDC connector, we also support custom properties with prefix i.e.: |
|
I think it’s a good idea to use a dedicated prefix for user-extensible properties to avoid potential key conflicts in the future. Instead of Examples: |
|
Personally, I prefer |
My question is: do we really need to define a prefix for custom properties? As you said, “from Fluss’s perspective, any property outside the table.* namespace is effectively a custom property.” When the Flink connector processes properties, it can filter out those custom ones and simply skip validating them. From this perspective, custom properties don’t necessarily need a prefix, right? After all, the Fluss server imposes no restrictions on custom properties, but the Flink read/write path introduces an additional constraint (and if users create tables via the API, they can completely bypass this constraint). |
Purpose
Linked issue: close #2425
Brief change log
Tests
API and Format
Documentation