Re: [PR] [FLINK-33784][table] Support Configuring CatalogStoreFactory via StreamExecutionEnvironment [flink]
xuyangzhong commented on code in PR #23990:
URL: https://github.com/apache/flink/pull/23990#discussion_r1522841798
##
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/factories/TableFactoryUtil.java:
##
@@ -237,15 +240,14 @@ public static CatalogStoreFactory
findAndCreateCatalogStoreFactory(
* }
*/
public static CatalogStoreFactory.Context buildCatalogStoreFactoryContext(
-Configuration configuration, ClassLoader classLoader) {
-String identifier =
configuration.get(CommonCatalogOptions.TABLE_CATALOG_STORE_KIND);
-String catalogStoreOptionPrefix =
-CommonCatalogOptions.TABLE_CATALOG_STORE_OPTION_PREFIX +
identifier + ".";
-Map options =
-new DelegatingConfiguration(configuration,
catalogStoreOptionPrefix).toMap();
-CatalogStoreFactory.Context context =
-new FactoryUtil.DefaultCatalogStoreContext(options,
configuration, classLoader);
-
-return context;
+ReadableConfig readableConfig, ClassLoader classLoader) {
+String identifier =
readableConfig.get(CommonCatalogOptions.TABLE_CATALOG_STORE_KIND);
+ConfigOption> catalogStoreMapOptions =
Review Comment:
My only concern is here, which actually brings weak constraints to the use
of options for CatalogStore (the value must be a string). Otherwise, an unknown
value will be retrieved without any exception being thrown.
After testing `configuration.setBytes(TABLE_CATALOG_STORE_OPTION_PREFIX +
"file.haha", new byte[] {1, 2});`, I found that the value became wrong.
I see that in jira, an agreement has been reached, so the choice is yours.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33784][table] Support Configuring CatalogStoreFactory via StreamExecutionEnvironment [flink]
hackergin commented on PR #23990: URL: https://github.com/apache/flink/pull/23990#issuecomment-1963168629 @twalthr @fsk119 Please help review when you have time, thank you. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33784][table] Support Configuring CatalogStoreFactory via StreamExecutionEnvironment [flink]
flinkbot commented on PR #23990: URL: https://github.com/apache/flink/pull/23990#issuecomment-1869049218 ## CI report: * da80787fb7b6e345b94278cbd74ce53a0bc6df6b UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-33784][table] Support Configuring CatalogStoreFactory via StreamExecutionEnvironment [flink]
hackergin opened a new pull request, #23990: URL: https://github.com/apache/flink/pull/23990 ## What is the purpose of the change The initialization of CatalogStore is currently before flink-conf.yaml and StreamTableEnvironment, which causes the configuration in flink-conf.yaml to not take effect. We should move the initialization of CatalogStore after the configuration merging. ## Brief change log * Postponed the discovery of CatalogStoreFactory until the final configuration is merged. * Using map type options for get CatalogStore confs ## Verifying this change Add unit test case in org.apache.flink.table.api.EnvironmentTest. Add CatalogStore Configuration in EnviromentSetting to verify if the discovery logic of CatalogStore can work properly, and whether other Enviroments can share the Catalog. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
