Airblader commented on a change in pull request #17332:
URL: https://github.com/apache/flink/pull/17332#discussion_r713951668
##########
File path:
flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/dialect/JdbcDialect.java
##########
@@ -142,4 +143,16 @@
*/
String getSelectFromStatement(
String tableName, String[] selectFields, String[] conditionFields);
+
+ /** Create catalog instance. */
+ default AbstractJdbcCatalog createCatalog(
+ String catalogName,
Review comment:
Basically, I think we should just pass the `context` argument from
`JdbcCatalogFactory#createCatalog` directly to `JdbcDialect#createCatalog`
instead of "destructuring" it into into a list of the individual options. Then
any changes to `CatalogFactory#Context` would also automatically become
available for dialect implementations.
I think we should also remove `username`, `pwd`, `baseUrl`, `defaultUrl`,
`getUsername`, `getPassword`, and `getBaseUrl` from `AbstractJdbcCatalog`.
Instead add a `abstract Connection getConnection()` method to
`AbstractJdbcCatalog` and use that in `#open`. This decouples the catalog
implementation from the concrete authentication implementation.
But I'm happy to hear other people's thoughts on this, too.
--
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
