danny0405 commented on code in PR #19772:
URL: https://github.com/apache/hudi/pull/19772#discussion_r3877429524
##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java:
##########
@@ -520,36 +520,19 @@ private static Boolean tableExists(Connection conn,
Map<String, String> options)
* @throws Exception
*/
public static HoodieSchema getJDBCSchema(Map<String, String> options) {
- Connection conn;
- String url;
- String table;
- boolean tableExists;
- try {
- conn = createConnection(options);
- url = options.get(JDBCOptions.JDBC_URL());
- table = options.get(JDBCOptions.JDBC_TABLE_NAME());
- tableExists = tableExists(conn, options);
- } catch (Exception e) {
- throw new HoodieSchemaFetchException("Failed to connect to jdbc", e);
- }
-
- if (!tableExists) {
- throw new HoodieSchemaFetchException(String.format("%s table does not
exists!", table));
- }
-
- try {
+ String url = options.get(JDBCOptions.JDBC_URL());
+ String table = options.get(JDBCOptions.JDBC_TABLE_NAME());
+ try (Connection conn = createConnection(options)) {
Review Comment:
Could we add a regression test that observes the `Connection` lifecycle
introduced here? The existing `TestJdbcbasedSchemaProvider` checks only the
returned schema (and currently catches/logs `HoodieException`), so it would
still pass if this leak regressed. A tracking test driver/connection could
assert `close()` after both a successful schema fetch and a failing
table/schema query; those are the two paths this change is meant to protect.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
