Airblader commented on a change in pull request #16214:
URL: https://github.com/apache/flink/pull/16214#discussion_r655109173
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/FactoryUtil.java
##########
@@ -578,15 +621,27 @@ public void validateExcept(String... prefixesToSkip) {
prefixesToSkip.length > 0, "Prefixes to skip can not be
empty.");
final List<String> prefixesList = Arrays.asList(prefixesToSkip);
consumedOptionKeys.addAll(
- configuration.keySet().stream()
+ allOptions.keySet().stream()
.filter(key ->
prefixesList.stream().anyMatch(key::startsWith))
.collect(Collectors.toSet()));
validate();
}
- /** Returns all options of the catalog. */
+ /** Returns all options currently being consumed by the factory. */
public ReadableConfig getOptions() {
- return configuration;
+ return allOptions;
+ }
+ }
+
+ /**
+ * Helper utility for validating all options for a {@link CatalogFactory}.
+ *
+ * @see #createCatalogFactoryHelper(CatalogFactory, CatalogFactory.Context)
+ */
+ public static class CatalogFactoryHelper extends
FactoryHelper<CatalogFactory> {
Review comment:
`@PublicEvolving`?
##########
File path:
flink-table/flink-table-common/src/test/java/org/apache/flink/table/factories/TestDynamicTableFactory.java
##########
@@ -50,10 +50,16 @@
public static final String IDENTIFIER = "test-connector";
public static final ConfigOption<String> TARGET =
- ConfigOptions.key("target").stringType().noDefaultValue();
+ ConfigOptions.key("target")
+ .stringType()
+ .noDefaultValue()
+ .withDeprecatedKeys("deprecated-target");
Review comment:
(Going on a bit of a tangent here, but we should eventually get to a
point where we can test these things in isolation rather than having a single
factory which tries to implement every test(able) scenario at once. Of course
this is out of scope for this PR.)
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/FactoryUtil.java
##########
@@ -596,32 +651,18 @@ public ReadableConfig getOptions() {
*
* @see #createTableFactoryHelper(DynamicTableFactory,
DynamicTableFactory.Context)
*/
- public static class TableFactoryHelper {
-
- private final DynamicTableFactory tableFactory;
+ public static class TableFactoryHelper extends
FactoryHelper<DynamicTableFactory> {
Review comment:
Unlike `CatalogFactoryHelper`, this wasn't `@PublicEvolving` before, but
it seems like it should be as well.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/FactoryUtil.java
##########
@@ -578,15 +621,27 @@ public void validateExcept(String... prefixesToSkip) {
prefixesToSkip.length > 0, "Prefixes to skip can not be
empty.");
final List<String> prefixesList = Arrays.asList(prefixesToSkip);
consumedOptionKeys.addAll(
- configuration.keySet().stream()
+ allOptions.keySet().stream()
.filter(key ->
prefixesList.stream().anyMatch(key::startsWith))
.collect(Collectors.toSet()));
validate();
}
- /** Returns all options of the catalog. */
+ /** Returns all options currently being consumed by the factory. */
public ReadableConfig getOptions() {
- return configuration;
+ return allOptions;
+ }
+ }
+
+ /**
+ * Helper utility for validating all options for a {@link CatalogFactory}.
+ *
+ * @see #createCatalogFactoryHelper(CatalogFactory, CatalogFactory.Context)
+ */
+ public static class CatalogFactoryHelper extends
FactoryHelper<CatalogFactory> {
Review comment:
Ah, I guess that makes sense. I went by the fact that it was annotated
previously already – which was done by me. :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
