-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-9172][sql-client] Support catalogs in SQL-Client yaml config file #8541
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
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe 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:
|
|
|
||
| private Catalog createCatalog(String name, Map<String, String> catalogProperties, ClassLoader classLoader) { | ||
| final CatalogFactory factory = | ||
| TableFactoryService.find(CatalogFactory.class, catalogProperties, classLoader); |
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.
Shall we rename the factory service, coz we are here creating catalogs rather than tables.
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 think it makes sense to rename the factory service and the factory interface itself to something more generic now as it's not only for 'table' anymore. I'll discuss with Timo, and it should be of its own PR.
| getCatalog("catalog2", "test"))); | ||
| } | ||
|
|
||
| private static Map<String, Object> getCatalog(String name, String type) { |
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 createCatalog or instantiateCatalog?
| * also {@link TableFactory} for more information. | ||
| */ | ||
| @PublicEvolving | ||
| public interface CatalogFactory extends TableFactory { |
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.
It's a little weird that CatalogFactory extends from TableFactory. Any thoughts?
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.
see above
xuefuz
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.
Looks mostly good. Some minor review comments.
[hotfix][table] minor refactor of CatalogDescriptor
|
@xuefuz thanks for your review. Can you take another look? |
|
The CI failure is due to flink-runtime is broken. Merging |
What is the purpose of the change
This PR adds support for basic catalog entries in SQL-Client yaml config file, adds
CatalogFactoryandCatalogDescriptor, and hooks them up with SQL Client thru table factory discovery service.Please refer to the design in doc.
Note that more fields of catalog entries and factories for existing catalogs will be added in the next few PRs.
This PR is a replacement of #7393
Brief change log
CatalogFactory,CatalogDescriptor,CatalogDescriptorValidatorin flink table modulesCatalogEntryand made SQL CLI handle catalog entries in yaml fileVerifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes)Documentation