You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/11/30 05:13:38 UTC

[GitHub] [flink-connector-jdbc] liuml07 opened a new pull request, #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

liuml07 opened a new pull request, #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4

   ## What is the purpose of the change
   
   Currently the `PostgresCatalog#listTables()` creates a new connection and prepared statement for every schema and table when listing tables. This can be optimized by reusing those resources.
   
   
   ## Brief change log
   
   *(for example:)*
     - Introduce `extractColumnValuesByStatement` in base class `AbstractJdbcCatalog`
     - Reuse connection and prepared in `PostgresCatalog#listTables()`
   
   This is moved from apache/flink#21146


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] MartijnVisser commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by GitBox <gi...@apache.org>.
MartijnVisser commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1351324792

   @liuml07 Could you please rebase?


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] eskabetxe commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "eskabetxe (via GitHub)" <gi...@apache.org>.
eskabetxe commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1563428418

   @MartijnVisser LGTM


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] dhruva1995 commented on a diff in pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "dhruva1995 (via GitHub)" <gi...@apache.org>.
dhruva1995 commented on code in PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#discussion_r1187885272


##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/AbstractJdbcCatalog.java:
##########
@@ -522,6 +509,25 @@ protected List<String> extractColumnValuesBySQL(
         }
     }
 
+    protected static List<String> extractColumnValuesByStatement(
+            PreparedStatement ps, int columnIndex, Predicate<String> filterFunc, Object... params)
+            throws SQLException {
+        List<String> columnValues = Lists.newArrayList();
+        if (Objects.nonNull(params) && params.length > 0) {
+            for (int i = 0; i < params.length; i++) {
+                ps.setObject(i + 1, params[i]);
+            }
+        }
+        ResultSet rs = ps.executeQuery();

Review Comment:
   `try(ResultSet rs = ps.executeQuery()) {...}`



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] MartijnVisser commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1563029609

   @eskabetxe Do you also want to review this PR?


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] liuml07 commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1561972204

   It's still a clean rebase and tests pass locally exception `OracleExactlyOnceSinkE2eTest` which fails on main as well in my IDE.


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] boring-cyborg[bot] commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1564107812

   Awesome work, congrats on your first merged pull request!
   


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] MartijnVisser merged pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser merged PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] liuml07 commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by GitBox <gi...@apache.org>.
liuml07 commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1352374675

   I found the `git fetch origin main && git rebase origin/main` has no conflicts. I just rebased and it still builds locally.


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] dhruva1995 commented on a diff in pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "dhruva1995 (via GitHub)" <gi...@apache.org>.
dhruva1995 commented on code in PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#discussion_r1187884474


##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/AbstractJdbcCatalog.java:
##########
@@ -522,6 +509,25 @@ protected List<String> extractColumnValuesBySQL(
         }
     }
 
+    protected static List<String> extractColumnValuesByStatement(
+            PreparedStatement ps, int columnIndex, Predicate<String> filterFunc, Object... params)
+            throws SQLException {
+        List<String> columnValues = Lists.newArrayList();
+        if (Objects.nonNull(params) && params.length > 0) {
+            for (int i = 0; i < params.length; i++) {
+                ps.setObject(i + 1, params[i]);
+            }
+        }
+        ResultSet rs = ps.executeQuery();

Review Comment:
   Nit Pick: though not related to your change but can you wrap the above line in try with resource block I think ResultSet also implements AutoClosable



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] MartijnVisser commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1561748232

   @liuml07 Can you please rebase once more and have a look at the new testing infrastructure for the connector, as setup by @eskabetxe ?


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] boring-cyborg[bot] commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1331653893

   Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)
   


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] liuml07 commented on pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#issuecomment-1561993108

   Tests seem to pass in Azure. I checked PR #22 which adds test extension and is a very good improvement. I think that is orthogonal to this change. The test extension is for improving testing infra to use one test container, and this change is for multiple similar queries to use one connection.


-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [flink-connector-jdbc] liuml07 commented on a diff in pull request #4: [FLINK-29750][Connector/JDBC] Improve PostgresCatalog#listTables() by reusing resources

Posted by "liuml07 (via GitHub)" <gi...@apache.org>.
liuml07 commented on code in PR #4:
URL: https://github.com/apache/flink-connector-jdbc/pull/4#discussion_r1187972768


##########
flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/catalog/AbstractJdbcCatalog.java:
##########
@@ -522,6 +509,25 @@ protected List<String> extractColumnValuesBySQL(
         }
     }
 
+    protected static List<String> extractColumnValuesByStatement(
+            PreparedStatement ps, int columnIndex, Predicate<String> filterFunc, Object... params)
+            throws SQLException {
+        List<String> columnValues = Lists.newArrayList();
+        if (Objects.nonNull(params) && params.length > 0) {
+            for (int i = 0; i < params.length; i++) {
+                ps.setObject(i + 1, params[i]);
+            }
+        }
+        ResultSet rs = ps.executeQuery();

Review Comment:
   Sounds good. Updated. thanks



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org