yuqi1129 commented on code in PR #13769:
URL: https://github.com/apache/gravitino/pull/13769#discussion_r4246486429
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java:
##########
@@ -79,6 +82,51 @@ public Capability newCapability() {
return new JdbcCatalogCapability();
}
+ /**
+ * Returns the effective {@link Catalog#PROPERTY_VERIFY_ON_CREATE
verify-on-create} setting for
+ * this catalog. A relational JDBC catalog verifies its backend at create
time by default; an
+ * administrator opts out by setting {@code
gravitino.catalog.verify-on-create=false} to register
+ * a catalog whose backend is temporarily unavailable.
+ *
+ * @return {@code true} to verify the backend at create (default); {@code
false} to skip the check
+ */
+ @Override
+ public boolean shouldValidateOnCreate() {
+ Map<String, String> properties = entity().getProperties();
+ if (properties == null) {
+ return true;
+ }
+ return
Boolean.parseBoolean(properties.getOrDefault(Catalog.PROPERTY_VERIFY_ON_CREATE,
"true"));
+ }
+
+ /**
+ * Verifies the backend is reachable at create time by running a single
{@code testConnection}
+ * (one {@code listDatabases} round trip) through the catalog operations. On
failure the backend
+ * raises a {@link ConnectionFailedException}, which propagates so {@code
CatalogManager} rolls
+ * back the just-persisted catalog entity.
+ *
+ * <p>No connection-pool leak: the probe borrows one connection and returns
it (JDBC operations
+ * use try-with-resources), and the pool stays lazy. If verification throws,
{@code
+ * CatalogManager.initCatalogWrapper} closes the partially-initialized
catalog, which closes the
+ * datasource and releases the pool.
+ */
+ @Override
+ public void validateOnCreate() {
Review Comment:
[Blocker] The create-time probe can now block indefinitely while holding the
metalake WRITE lock.
Since 55ccb5db3, only the connect timeout is injected. If the endpoint
accepts the TCP connection but never speaks the database protocol, the driver's
handshake read waits on `socketTimeout`, which defaults to 0 (no timeout). This
happens with a load balancer whose backend is down, a wrong port that lands on
another service, or a proxy that accepts and doesn't forward.
Reproduced with a `ServerSocket` that accepts and stays silent:
- MySQL Connector/J 8.0.33 with only `connectTimeout=2000`: still blocked
after 20 s.
- pgjdbc 42.7.11 with only `connectTimeout=2`: still blocked after 20 s.
This probe runs inside `TreeLockUtils.doWithTreeLock(metalakeIdent,
LockType.WRITE, ...)` (CatalogManager.java:912). TreeLock takes READ on every
ancestor with an untimed `lock()` (TreeLock.java:103, TreeLockNode.java:150).
So one hung create blocks every catalog, schema and table operation in that
metalake, with no upper bound. Before this PR, create never touched the
backend, so this failure mode is new.
Suggestion: bound only the probe, not the pooled connections, which keeps
the fix for normal operations. In the same probe:
- MySQL with `socketTimeout=2000` failed after 2032 ms.
- PostgreSQL with `loginTimeout=2` failed after 2012 ms, and with
`socketTimeout=2` after 4009 ms.
For example, open a one-off probe connection through `Driver.connect` with
those properties. Better still, run the probe outside the lock, before
`store.put`, on a temporary wrapper as the standalone `testConnection` does. A
unit test with an accept-but-silent server would pin it.
Not verified: the ClickHouse driver.
##########
catalogs/catalog-jdbc-mysql/src/test/java/org/apache/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java:
##########
@@ -216,6 +216,10 @@ void
testExistingCatalogConnectionWithUnreachableEndpoint() throws SQLException
JdbcConfig.JDBC_DRIVER.getKey(),
MYSQL_CONTAINER.getDriverClassName(TEST_DB_NAME));
catalogProperties.put(JdbcConfig.USERNAME.getKey(),
MYSQL_CONTAINER.getUsername());
catalogProperties.put(JdbcConfig.PASSWORD.getKey(),
MYSQL_CONTAINER.getPassword());
+ // This test registers a catalog pointing at an unreachable endpoint on
purpose, so it can then
+ // assert testConnection fails on the EXISTING catalog. It must opt out of
create-time
+ // verification; the rejection by the default create-time verification is
covered separately.
Review Comment:
[Nit] I can't find the separate coverage this comment refers to. The only
test of the default rejection is the mocked one in `TestCatalogManager`. No IT
creates a JDBC catalog against a real driver and asserts the rejection.
`catalog-jdbc-common` also has no unit test for:
- `JdbcCatalog.shouldValidateOnCreate` / `validateOnCreate`;
- the host-context wrapping in `testConnection`;
- the standalone `testConnection` probing only once.
Please add the tests, or adjust this comment.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java:
##########
@@ -114,6 +114,26 @@ public class JdbcConfig extends Config {
.checkValue(value -> value > 0,
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
.createWithDefault(30000L);
+ /**
+ * The canonical connection-establishment timeout for a single JDBC
connection, in milliseconds.
+ * This bounds how long establishing a connection may block against an
unreachable backend, so
+ * that create-time verification fails fast instead of hanging. It is
translated to each driver
+ * family's own connect-timeout parameter (and unit) by {@link
+ * org.apache.gravitino.catalog.jdbc.utils.JdbcConnectionTimeouts}; only the
connect timeout is
+ * bounded (never the socket-read timeout, which would also abort legitimate
long-running
+ * operations), and an operator value set directly on the connection or in
the {@code jdbc-url} is
+ * never overridden.
+ */
+ public static final ConfigEntry<Long> CONNECT_TIMEOUT_MS =
+ new ConfigBuilder("jdbc.pool.connect-timeout-ms")
+ .doc(
+ "The connection-establishment timeout for a single JDBC
connection, in "
+ + "milliseconds, translated per driver so an unreachable
backend fails fast")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .longConf()
+ .checkValue(value -> value > 0,
ConfigConstants.POSITIVE_NUMBER_ERROR_MSG)
Review Comment:
[Nit] The overflow part of the earlier comment is fixed, with a test. The
range part is not: the MySQL and PostgreSQL timeouts are Java `int`s, so a
value above `Integer.MAX_VALUE` milliseconds, or `Integer.MAX_VALUE` seconds
after conversion for PostgreSQL, is accepted here and only fails when the
driver parses it at connect. An upper bound in this `checkValue` would reject
it when the config is validated. I didn't check the exact driver error.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -996,7 +996,11 @@ public void testConnection(
.build())
.build();
- CatalogWrapper wrapper = createCatalogWrapper(dummyEntity, mergedConfig);
+ // Pass null propsToValidate so wrapper construction does NOT run the
create-time backend
+ // validation (BaseCatalog#validateOnCreate). This standalone path
performs its own explicit
+ // probe below; letting the wrapper validate as well would probe the
backend twice and could
+ // fail an otherwise-successful test on a transient second-round-trip
error.
+ CatalogWrapper wrapper = createCatalogWrapper(dummyEntity, null);
Review Comment:
[Important] Passing `null` here also disables property validation, which is
a regression against main.
`propsToValidate` controls both `validatePropertyForCreate` (line 1827) and
`validateOnCreate` (line 1837), and main passes `mergedConfig` here. I added a
temporary test to `TestCatalogManager`:
- On this head, `testConnection` with the reserved `reserved_key` succeeds,
and so does `testConnection` without the required `key1`.
- With `mergedConfig` restored, they throw `IllegalArgumentException:
Properties or property prefixes are reserved and cannot be set: [reserved_key]`
and `... are required and must be set: [key1]`.
`HiddenPropertyMaskUtils.validateNoMaskedPlaceholders` is skipped as well.
So `testConnection` can report success for properties that `createCatalog`
rejects.
Suggestion: keep property validation and skip only the backend probe, for
example `createCatalogWrapper(entity, propsToValidate, boolean
validateBackend)`. Add both cases above as regression tests.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java:
##########
@@ -244,16 +244,67 @@ public void testConnection(NameIdentifier catalogIdent) {
try {
databaseOperation.listDatabases();
} catch (ConnectionFailedException e) {
- throw e;
+ // Already classified as a connection failure (for example a MySQL
access-denied or a
+ // PostgreSQL SQLSTATE 08 error). Re-wrap it with the host context so
every connection failure
+ // satisfies the documented "names the host" contract, preserving the
original cause.
+ throw new ConnectionFailedException(
+ e.getCause() != null ? e.getCause() : e,
+ "Failed to connect to JDBC catalog at %s: %s",
+ hostPortFromJdbcUrl(jdbcUrl),
+ e.getMessage());
} 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 {@code "unknown"} whenever the
host/port cannot be located
+ * (including a URL with no {@code ://} authority) rather than the raw URL,
because the raw URL
+ * can carry credentials (for example {@code
jdbc:postgresql:db?password=...}) that must not leak
+ * into the exception, the REST response, or the server logs. 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, or {@code "unknown"} when it
cannot be extracted
+ */
+ private static String hostPortFromJdbcUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return "unknown";
+ }
+ int schemeEnd = url.indexOf("://");
+ if (schemeEnd < 0) {
+ return "unknown";
+ }
+ int authorityStart = schemeEnd + "://".length();
+ // The authority ends at the first '/', '?', or ';' following it.
+ int authorityEnd = url.length();
+ for (int i = authorityStart; i < url.length(); i++) {
+ char c = url.charAt(i);
+ if (c == '/' || c == '?' || c == ';') {
+ authorityEnd = i;
+ break;
+ }
+ }
+ String authority = url.substring(authorityStart, authorityEnd);
+ if (StringUtils.isBlank(authority)) {
+ return "unknown";
+ }
+ // Strip any user-info ("user@host:port"); keep only the host:port part.
+ int at = authority.lastIndexOf('@');
+ if (at >= 0) {
+ authority = authority.substring(at + 1);
+ }
+ return StringUtils.isBlank(authority) ? "unknown" : authority;
Review Comment:
[Important] The raw-URL leak is fixed for the fallback branches, but this
branch still returns credentials for MySQL's key-value host syntax.
I called this method by reflection on 706c91607a:
- `jdbc:mysql://(host=db1,port=3306,user=root,password=s3cret)/test` returns
`(host=db1,port=3306,user=root,password=s3cret)`.
- `jdbc:mysql://address=(host=db1)(port=3306)(password=s3cret)/test` returns
the address part unchanged, password included.
Connector/J accepts both forms, so the password ends up in the REST error
and in the server logs, the same consequence as the earlier finding.
`user:pw@host` and URLs without `://` are handled correctly.
Suggestion: accept only an authority that matches `host[:port]` (including
bracketed IPv6) and return `unknown` for anything else. Add tests for these two
forms and for the `unknown` fallbacks, which currently have no test.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/JdbcConnectionTimeouts.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.jdbc.utils;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Translates the canonical {@link JdbcConfig#CONNECT_TIMEOUT_MS
connect-timeout-ms} into the
+ * driver-family connection properties that actually bound a JDBC connection
attempt.
+ *
+ * <p>DBCP has no portable TCP-connect timeout; the real bound lives in the
driver, and both the
+ * parameter name and its unit differ per driver family:
+ *
+ * <ul>
+ * <li><b>MySQL Connector/J</b> ({@code com.mysql.cj.jdbc.Driver}, the
legacy {@code
+ * com.mysql.jdbc.Driver} — also used by Doris and StarRocks — and the
MySQL-compatible
+ * OceanBase Connector/J {@code com.oceanbase.jdbc.Driver}): {@code
connectTimeout} and {@code
+ * socketTimeout} in <b>milliseconds</b>.
+ * <li><b>PostgreSQL</b> ({@code org.postgresql.Driver}, also used by the
PostgreSQL-compatible
+ * Hologres catalog): {@code connectTimeout} and {@code socketTimeout}
in <b>seconds</b>.
+ * <li><b>ClickHouse</b> ({@code com.clickhouse.jdbc.ClickHouseDriver}):
{@code connect_timeout}
+ * and {@code socket_timeout} in <b>milliseconds</b> (note the
underscore names).
+ * </ul>
+ *
+ * <p>An unknown driver family yields no properties (logged at debug), so
adding a new JDBC provider
+ * never breaks; that provider opts in by registering its family here.
+ *
+ * <p><b>Defaults, never overrides.</b> For each property a family would emit,
an operator value is
+ * preserved when it is already present either as a DBCP connection property
key in the catalog
+ * configuration ({@link JdbcConfig#getAllConfig()}) or as a query parameter
in the {@code
+ * jdbc-url}. In that case the property is omitted from the returned map so
the operator's value
+ * wins. The URL-parameter check is case-sensitive and anchored at a parameter
boundary ({@code ?}
+ * or {@code &}), mirroring {@code MaxComputeTimeoutUtils.hasParameter}, so a
differently-cased or
+ * substring variant does not suppress injection of the canonical key.
+ *
+ * <p>The returned properties are applied as DBCP <em>connection</em>
properties (via {@code
+ * BasicDataSource.addConnectionProperty}), not raw pool keys, so they neither
collide with the
+ * unsafe pool properties nor make the pool eager; they take effect on the
first real connection,
+ * i.e. during create-time verification.
+ */
+public final class JdbcConnectionTimeouts {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(JdbcConnectionTimeouts.class);
+
+ /** The MySQL Connector/J driver class (modern). */
+ static final String MYSQL_DRIVER = "com.mysql.cj.jdbc.Driver";
+
+ /** The MySQL Connector/J driver class (legacy, pre-8.0). */
+ static final String MYSQL_LEGACY_DRIVER = "com.mysql.jdbc.Driver";
+
+ /** The OceanBase Connector/J driver class (MySQL-compatible, milliseconds).
*/
+ static final String OCEANBASE_DRIVER = "com.oceanbase.jdbc.Driver";
+
+ /** The PostgreSQL driver class (also used by the Hologres catalog). */
+ static final String POSTGRESQL_DRIVER = "org.postgresql.Driver";
+
+ /** The ClickHouse JDBC driver class. */
+ static final String CLICKHOUSE_DRIVER =
"com.clickhouse.jdbc.ClickHouseDriver";
+
+ /** The connect/socket parameter names used by the MySQL and PostgreSQL
families (camelCase). */
+ static final String CONNECT_TIMEOUT_PARAM = "connectTimeout";
+
+ static final String SOCKET_TIMEOUT_PARAM = "socketTimeout";
+
+ /** The connect/socket parameter names used by ClickHouse (snake_case). */
+ static final String CLICKHOUSE_CONNECT_TIMEOUT_PARAM = "connect_timeout";
+
+ static final String CLICKHOUSE_SOCKET_TIMEOUT_PARAM = "socket_timeout";
+
+ private JdbcConnectionTimeouts() {
+ // Utility class, prevent instantiation.
+ }
+
+ /**
+ * Returns the driver-family connection properties that bound a single
connection to the canonical
+ * {@link JdbcConfig#getConnectTimeoutMs()} value, omitting any the operator
has already set.
+ *
+ * @param config the catalog JDBC configuration; must not be null
+ * @param jdbcUrl the JDBC URL whose query parameters are inspected for
operator-set timeouts; may
+ * be null or blank
+ * @return an ordered map of connection property name to value; empty when
the driver family is
+ * unknown or the operator has already set every relevant property
+ */
+ public static Map<String, String> connectionProperties(JdbcConfig config,
String jdbcUrl) {
+ Map<String, String> properties = new LinkedHashMap<>();
+ String driver = config.getJdbcDriver();
+ long millis = config.getConnectTimeoutMs();
+
+ String connectParam;
+ String socketParam;
+ boolean seconds;
+ if (MYSQL_DRIVER.equals(driver)
+ || MYSQL_LEGACY_DRIVER.equals(driver)
+ || OCEANBASE_DRIVER.equals(driver)) {
+ // MySQL Connector/J and the MySQL-compatible OceanBase driver expect
milliseconds.
+ connectParam = CONNECT_TIMEOUT_PARAM;
+ socketParam = SOCKET_TIMEOUT_PARAM;
+ seconds = false;
+ } else if (POSTGRESQL_DRIVER.equals(driver)) {
+ // PostgreSQL (and Hologres via the PostgreSQL driver) expect seconds.
+ connectParam = CONNECT_TIMEOUT_PARAM;
+ socketParam = SOCKET_TIMEOUT_PARAM;
+ seconds = true;
+ } else if (CLICKHOUSE_DRIVER.equals(driver)) {
+ // ClickHouse uses snake_case parameter names and expects milliseconds.
+ connectParam = CLICKHOUSE_CONNECT_TIMEOUT_PARAM;
+ socketParam = CLICKHOUSE_SOCKET_TIMEOUT_PARAM;
+ seconds = false;
+ } else {
+ LOG.debug(
+ "JDBC driver '{}' has no known connect-timeout translation; leaving
connection timeouts "
+ + "unbounded by Gravitino. Set them in the jdbc-url if needed.",
+ driver);
+ return properties;
+ }
+
+ String value = Long.toString(seconds ? millisToSeconds(millis) : millis);
+ addIfAbsent(properties, connectParam, value, config, jdbcUrl);
+ addIfAbsent(properties, socketParam, value, config, jdbcUrl);
+ return properties;
+ }
+
+ /**
+ * Converts milliseconds to seconds, rounded up with a minimum of 1 so a
sub-second bound never
+ * collapses to 0 ("no timeout").
+ *
+ * @param millis the timeout in milliseconds
+ * @return the timeout in whole seconds, at least 1
+ */
+ private static long millisToSeconds(long millis) {
+ return Math.max(1L, (millis + 999L) / 1000L);
+ }
+
+ /**
+ * Adds a connection property unless the operator has already set it (as a
config key or a {@code
+ * jdbc-url} query parameter), preserving the operator's value in that case.
+ *
+ * @param properties the map being built
+ * @param name the connection property name
+ * @param value the Gravitino default value
+ * @param config the catalog configuration
+ * @param jdbcUrl the JDBC URL
+ */
+ private static void addIfAbsent(
+ Map<String, String> properties,
+ String name,
+ String value,
+ JdbcConfig config,
+ String jdbcUrl) {
+ if (config.getAllConfig().containsKey(name) || hasUrlParameter(jdbcUrl,
name)) {
+ return;
+ }
+ properties.put(name, value);
Review Comment:
Still open on 706c91607a. `addIfAbsent` still checks only
`config.getAllConfig().containsKey(name)`, and `DataSourceUtils.java:174` is
unchanged. Dropping `socketTimeout` makes the problem smaller, but
`connectTimeout` is still affected in both directions:
- A `connectTimeout` the operator set inside the DBCP `connectionProperties`
value is not detected, so it gets overwritten.
- A raw `gravitino.bypass.connectTimeout` key suppresses the injection, but
DBCP ignores that key, so the connection ends up with no timeout at all.
Please merge the default into the data source's actual connection
properties, and add tests for both forms.
--
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]
