jerryshao commented on code in PR #13729:
URL: https://github.com/apache/gravitino/pull/13729#discussion_r4236869240
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -754,21 +808,46 @@ public Integer calculateDatetimePrecision(String
typeName, int columnSize, int s
}
/**
- * Get MySQL driver version from DatabaseMetaData
+ * Get MySQL driver version from DatabaseMetaData.
+ *
+ * <p>The version is cached once read. {@link #load} caches it from the
connection it holds before
+ * parsing columns (see {@link #cacheDriverVersion}), so this method borrows
a connection only
+ * when nothing has cached it yet.
*
* @return the driver version string, or null if not available
*/
protected String getMySQLDriverVersion() {
+ if (driverVersionCached || dataSource == null) {
+ return driverVersion;
+ }
+ try (Connection connection = dataSource.getConnection()) {
+ cacheDriverVersion(connection);
+ } catch (SQLException e) {
+ LOG.debug("Failed to get driver version", e);
+ }
+ return driverVersion;
+ }
+
+ /**
+ * Caches the driver version from a connection the caller already holds.
+ *
+ * <p>Overrides of {@link #load} that parse columns must call this first, so
that {@link
+ * #getMySQLDriverVersion()} does not borrow another connection while theirs
is held.
+ *
+ * @param connection the connection held by the caller
Review Comment:
[Nit] The contract added here is not met by the three in-tree `load`
overrides that parse columns: `PostgreSqlTableOperations.load`
(catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:102,
columns at :149), `HologresTableOperations.load` (:138, columns at :185) and
`ClickHouseTableOperations.load`
(catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:883,
columns at :901) never call `cacheDriverVersion`.
That is harmless today only because none of those three reads the driver
version while parsing a column: PostgreSQL (:768) and Hologres (:755) override
`calculateDatetimePrecision` without it, and ClickHouse inherits the base one
(JdbcTableOperations:803) that returns null. As soon as one of them adds a
version check, the per-column nested borrow this PR removes comes back, and the
Javadoc gives no hint about which overrides need updating.
Suggest either adding the call to those three overrides now, or caching the
version where no override can miss it - once in `initialize`, or in
`getConnection(String)` (JdbcTableOperations:724), which already holds a
connection and is on every path.
Verified by: read all four `load` implementations and all five
`calculateDatetimePrecision` overrides in the clone; `getBasicJdbcColumnInfo`
(:737-744) is the only caller of `calculateDatetimePrecision`, and each of the
four load paths reaches it.
##########
catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/ConnectionCountingDataSource.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * 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.lang.reflect.Proxy;
+import java.sql.Connection;
+import java.sql.DatabaseMetaData;
+import java.sql.Statement;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.sql.DataSource;
+
+/**
+ * A test {@link DataSource} whose connections only record the SQL they
execute. It tracks how many
+ * connections are borrowed at once, so tests can assert that an operation
never holds one
+ * connection while borrowing another.
+ */
+public class ConnectionCountingDataSource {
+
+ private final AtomicInteger borrowed = new AtomicInteger();
+ private final AtomicInteger peakBorrowed = new AtomicInteger();
+ private final AtomicInteger totalBorrows = new AtomicInteger();
+ private final List<String> executedSql = new CopyOnWriteArrayList<>();
+ private final DataSource dataSource =
+ (DataSource)
+ Proxy.newProxyInstance(
+ DataSource.class.getClassLoader(),
+ new Class<?>[] {DataSource.class},
+ (proxy, method, args) -> {
+ if ("getConnection".equals(method.getName())) {
+ return borrow();
+ }
+ return objectMethod(proxy, method.getName(), args,
"DataSource");
+ });
+
+ /** Returns the data source to pass to the operations under test. */
+ public DataSource dataSource() {
+ return dataSource;
+ }
+
+ /** Returns the number of connections currently borrowed and not yet closed.
*/
+ public int borrowed() {
+ return borrowed.get();
+ }
+
+ /** Returns the largest number of connections that were borrowed at the same
time. */
+ public int peakBorrowed() {
+ return peakBorrowed.get();
+ }
+
+ /** Returns how many connections were borrowed in total. */
+ public int totalBorrows() {
+ return totalBorrows.get();
+ }
+
+ /** Returns the update statements executed so far, in order. */
+ public List<String> executedSql() {
+ return executedSql;
+ }
+
+ private Connection borrow() {
+ totalBorrows.incrementAndGet();
+ peakBorrowed.accumulateAndGet(borrowed.incrementAndGet(), Math::max);
+ AtomicBoolean closed = new AtomicBoolean();
+ return (Connection)
+ Proxy.newProxyInstance(
+ Connection.class.getClassLoader(),
+ new Class<?>[] {Connection.class},
+ (proxy, method, args) -> {
+ switch (method.getName()) {
+ case "close":
+ if (closed.compareAndSet(false, true)) {
+ borrowed.decrementAndGet();
+ }
Review Comment:
[Nit] This proxy answers only `close`, `setCatalog`, `setSchema` and
`createStatement`, so `getMetaData` falls through to `objectMethod` and throws
`UnsupportedOperationException`. Since base `load` now calls
`cacheDriverVersion(connection)` then `connection.getMetaData()`
(JdbcTableOperations:256 and :842), this data source can no longer be used to
exercise `load` at all - which is why the load and alter reproductions in
`TestJdbcOperationsConnectionUsage` fall back to a real single-connection
SQLite pool, and the `peakBorrowed()` assertions that cover the other paths are
missing for the load fix.
Returning a `DatabaseMetaData` proxy that answers `getDriverVersion` (and
throws for everything else, as now) would let the load path be covered with the
same borrow counting as `create`, `alterTable` and `dropDatabase`.
Verified by: traced the switch default in this proxy; `load` reaches
`cacheDriverVersion` at JdbcTableOperations:256 and `getMetaData()` at :842, so
any `load` against this data source fails with `UnsupportedOperationException`
rather than a counted borrow.
--
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]
