[FLINK-17748][table] Remove registration of TableSource/TableSink in Table Env #12076
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 848b688 (Fri Oct 16 10:31:28 UTC 2020) 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:
|
|
@KurtYoung Could you have a look at this if you have time? |
|
@flinkbot run azure |
| String name, | ||
| TableSource<?> tableSource, | ||
| boolean isBatch) { | ||
| if (tEnv instanceof TableEnvironmentInternal) { |
There was a problem hiding this comment.
check and throw exception first, and do other normal logic without this big brace
| TableSource<?> tableSource, | ||
| boolean isBatch) { | ||
| if (tEnv instanceof TableEnvironmentInternal) { | ||
| CatalogManager catalogManager = ((TableEnvironmentInternal) tEnv).getCatalogManager(); |
There was a problem hiding this comment.
You can keep all the logic of original registerTableSourceInternal, e.g. it will call validateTableSource when register
| * Register a table source as a Table under the given name in this | ||
| * {@link TableEnvironment}'s catalog. | ||
| */ | ||
| public static void registerTableSource( |
There was a problem hiding this comment.
registerTableSourceInternal
| * Register a table sink as a Table under the given name in this | ||
| * {@link TableEnvironment}'s catalog. | ||
| */ | ||
| public static void registerTableSink( |
There was a problem hiding this comment.
registerTableSinkInternal
| * Utility methods to register table source/sink as a Table in {@link TableEnvironment}'s catalog. | ||
| */ | ||
| @Internal | ||
| public class TableEnvUtils { |
There was a problem hiding this comment.
How about adding these two methods to TableEnvironmentInternal?
There was a problem hiding this comment.
OK. I will add registerTableSource and registerTableSink to TableEnvironmentInternal .
| * @param name The name under which the {@link TableSource} is registered. | ||
| * @param tableSource The {@link TableSource} to register. | ||
| */ | ||
| void registerTableSource(String name, TableSource<?> tableSource); |
There was a problem hiding this comment.
Be consistent with other internal methods, add a Internal suffix
KurtYoung
changed the title
|
I fixed the compilation error and python tests, AZP: https://dev.azure.com/ykt836/Flink/_build/results?buildId=7&view=results |
What is the purpose of the change
This PR is a workaround to remove all registration of TableSource/TableSink in Table Env. We introduces utility methods to register TableSource/TableSink since descriptor APIs is not enough to fulfill some testing scenarios.
Brief change log
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation