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-23064][flink-connectors][flink-formats] Expose connector/format config options as @PublicEvolving #16334
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit f202241 (Wed Jun 30 12:13:01 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
9b27c8b
to
8088724
Compare
34987c6
to
5a7928b
Compare
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.
Thanks for the PR @Airblader. I added a bunch of comments to further ensure consistency. It is very helpful to see all options at once in such a PR to unify them further.
...ain/java/org/apache/flink/streaming/connectors/elasticsearch/table/ElasticsearchOptions.java
Outdated
Show resolved
Hide resolved
|
||
/** Utilities for {@link HBaseOptions}. */ | ||
@Internal | ||
public class HBaseOptionsUtil { |
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.
is default scope possible for these utilities that should always be next to the factory in the same package?
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.
I made it default scope where possible for now, but it isn't possible for all.
return hbaseClientConf; | ||
} | ||
|
||
private static Properties getHBaseClientProperties(Map<String, String> tableOptions) { |
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.
could we add a [hotfix]
commit to replace this prefix magic? We can replace this with a map option type nowadays. Also for the Kafka connector and maybe also for the Kinesis connector. Otherwise the new Option classes are not very useful if they are less complete.
...rs/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcOptions.java
Outdated
Show resolved
Hide resolved
...rs/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcOptions.java
Outdated
Show resolved
Hide resolved
...stry/src/main/java/org/apache/flink/formats/avro/registry/confluent/RegistryAvroOptions.java
Outdated
Show resolved
Hide resolved
...stry/src/main/java/org/apache/flink/formats/avro/registry/confluent/RegistryAvroOptions.java
Outdated
Show resolved
Hide resolved
|
||
/** Utilities for {@link DataGenOptions}. */ | ||
@Internal | ||
public class DataGenOptionsUtil { |
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.
Actually, the task of the Factory
is to translate options. We could also get rid of most of util classes and move the code to the factory itself.
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.
For DataGen I'm hesitant to do so because the factory is marked as PublicEvolving (and these would have to be visible because they're accessed from other places, too). Most other factories (of "real connectors") are marked Internal, though. I'm not sure if we can just changes this to Internal now? If not I'd probably keep it separate for now.
@PublicEvolving | ||
public class RawOptions { | ||
|
||
public static final String BIG_ENDIAN = "big-endian"; |
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.
we should improve our ConfigOption
design to allow enums with special characters, it doesn't help if we support enum type but nobody wants to use them and they use string type again
...ctor-kafka/src/main/java/org/apache/flink/streaming/connectors/kafka/table/KafkaOptions.java
Outdated
Show resolved
Hide resolved
Thanks for the review @twalthr. I think all your points are valid, but I'd like to address the rest of them in a separate PR. To summarize:
|
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.
Some last minor feedback. Good to merge in the next iteration. Let me know if you have time otherwise I can also perform them during merging.
.defaultValue(3) | ||
.withDescription("the max retry times if lookup database failed."); | ||
|
||
public static final ConfigOption<Integer> SINK_PARALLELISM = FactoryUtil.SINK_PARALLELISM; |
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.
private constructor is missing
import org.apache.flink.configuration.ConfigOption; | ||
import org.apache.flink.configuration.ConfigOptions; | ||
import org.apache.flink.connector.jdbc.catalog.JdbcCatalog; | ||
import org.apache.flink.table.catalog.CommonCatalogOptions; | ||
|
||
/** {@link ConfigOption}s for {@link JdbcCatalog}. */ | ||
@Internal | ||
@PublicEvolving |
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.
rename to JdbcCatalogOptions
? or leave it @Internal
for now
FAIL, | ||
DROP, | ||
LITERAL | ||
} |
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.
private constructor missing
@@ -741,8 +550,9 @@ private static boolean hasKafkaClientProperties(Map<String, String> tableOptions | |||
* Creates an array of indices that determine which physical fields of the table schema to | |||
* include in the value format. | |||
* | |||
* <p>See {@link #VALUE_FORMAT}, {@link #VALUE_FIELDS_INCLUDE}, and {@link #KEY_FIELDS_PREFIX} | |||
* for more information. | |||
* <p>See {@link KafkaConnectorOptions#VALUE_FORMAT}, {@link |
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.
We could have avoided these changes with static imports.
…fig options as PublicEvolving
What is the purpose of the change
This exposes
ConfigOption<>
s for connectors and formats by marking them as@PublicEvolving
. Where applicable we reduce the exposed API surface by refactoring constants, validation helpers, and utilities into another class (annotated with@Internal
).This is done as part of FLIP-129 to allow users to refer to these options when creating table (and format) descriptors.
(!) Review on a per-commit basis is encouraged. Each commit touches a specific connector/format. Otherwise the changes are fairly technical.
In a follow-up step (separate sub-task) we'll move these classes into a common location (package) for easier discovery.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation