[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r720671425 ## 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: Yes, you are right. But I dont understand why you have to refactor `Context` on this PR. This PR is designed to support custom calalogs :-) -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r720671425 ## 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: Yes, you are right. But I dont understand why you have to refactor the code on this PR. This PR is designed to support custom calalogs :-) -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r715475502 ## 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: > Otherwise the other ticket will fall through the cracks and this gets released as-is. I'm not a committer, though, this is just my opinion. i am sorry, i dont understand why can not we merge the PR first and then create a new ticket? -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r714613859 ## 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. thx @Airblader got it. but i prefer to create a new refactored issue -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713691969 ## 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: > But you are adding a new public API method to this interface in this PR. So we need to make sure that the design of this method is good. sorry, i dont understand how I should do it... Can you go into more detail? -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713672529 ## 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: but the JdbcDialect already has `@PublicEvolving`, https://github.com/apache/flink/blob/d753cf20826c66f20bfbd3772c58709e00d9bcb7/flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/dialect/JdbcDialect.java#L35 -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713664342 ## 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: ok，i prefer to create new issue for `Context`. of course, if u think it is better to update `Context` in the patch, i can update the patch -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713664342 ## 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: ok，i prefer to create new issue for `Context`. of course, if u think it is better to update `Context` in the patch, i can modify it -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713631422 ## 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: u mean we should revise from here `https://github.com/apache/flink/blob/d753cf20826c66f20bfbd3772c58709e00d9bcb7/flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/factory/JdbcCatalogFactory.java#L72` ? -- 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
[GitHub] [flink] cuibo01 commented on a change in pull request #17332: [FLINK-24349]Support customized Calalogs via JDBC
cuibo01 commented on a change in pull request #17332: URL: https://github.com/apache/flink/pull/17332#discussion_r713626650 ## 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( Review comment: thx for your review... yes, u are right. If we add new interface, we need to modify a lot code. I think the current way might be better than modifying a lot of code. -- 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
