yuqi1129 commented on code in PR #13769:
URL: https://github.com/apache/gravitino/pull/13769#discussion_r4245516439
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java:
##########
@@ -167,6 +167,12 @@ private static DataSource createDBCPDataSource(JdbcConfig
jdbcConfig) throws Exc
// Validate connections on borrow when enabled.
basicDataSource.setTestOnBorrow(jdbcConfig.getTestOnBorrow());
basicDataSource.setMaxWait(Duration.ofMillis(jdbcConfig.getMaxWaitMs()));
+ // Apply the driver-aware connect/socket timeout as DBCP connection
properties (not pool keys,
+ // so no collision with UNSAFE_POOL_PROPERTIES and the pool stays lazy).
These bound a single
+ // connection and take effect on the first real connection — i.e.
create-time verification — so
+ // an unreachable backend fails fast instead of hanging. Operator-set
values are preserved.
+ JdbcConnectionTimeouts.connectionProperties(jdbcConfig, jdbcUrl)
Review Comment:
[Important] Separate probe timeouts from normal operation timeouts
This also installs the default socket timeout on every ordinary JDBC
connection, including connections for existing catalogs. Normal operations such
as `ALTER TABLE` can therefore start failing after 30 seconds without a
response, and `verify-on-create=false` does not restore their previous
behavior. Please separate connect/read/probe timeout settings, or explicitly
document and test this compatibility change. A bounded create-time probe should
not silently impose a new timeout on normal operations.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java:
##########
@@ -248,12 +248,54 @@ public void testConnection(NameIdentifier catalogIdent) {
} catch (GravitinoRuntimeException e) {
if (e.getClass() == GravitinoRuntimeException.class && e.getCause()
instanceof SQLException) {
throw new ConnectionFailedException(
- e.getCause(), "Failed to connect to JDBC catalog: %s",
e.getMessage());
+ e.getCause(),
+ "Failed to connect to JDBC catalog at %s: %s",
+ hostPortFromJdbcUrl(jdbcUrl),
+ e.getMessage());
}
throw e;
}
}
+ /**
+ * Extracts {@code host:port} from a {@code
jdbc:subprotocol://host:port/...} URL for use in a
+ * connection-failure message. Returns the raw URL when the host/port cannot
be located and {@code
+ * "unknown"} when the URL is blank. Never throws, so message formatting
cannot mask the original
+ * connection failure.
+ *
+ * @param url the JDBC URL stored for this catalog
+ * @return a {@code host:port} description, the raw URL, or {@code "unknown"}
+ */
+ private static String hostPortFromJdbcUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return "unknown";
+ }
+ int schemeEnd = url.indexOf("://");
+ if (schemeEnd < 0) {
+ return url;
Review Comment:
[Blocker] Never return the raw JDBC URL in error formatting
Returning the raw URL when host extraction fails can expose credentials in
the exception, REST response, and server logs. For example, the PostgreSQL
driver accepts `jdbc:postgresql:database?password=…`, but this fallback
includes the password in the resulting connection-failure message. A targeted
probe with the real PostgreSQL driver reproduced this. Please return a safe
placeholder such as `unknown` when extraction fails, and apply the same rule to
every fallback.
--
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]
