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`. We
can either move the connection test logic of `#open` into `JdbcCatalogUtils`
and call it from `PostgresCatalog`, or we 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
