yuqi1129 commented on code in PR #13729:
URL: https://github.com/apache/gravitino/pull/13729#discussion_r4236963350
##########
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:
Fixed in da228c71e3: ClickHouse's independent `load` now calls
`cacheDriverVersion(connection)` before parsing columns, and its actual-load
unit test verifies that metadata read.
On the current head, PostgreSQL's line 102 and Hologres's line 138 are
`listTables`, not `load`; both inherit the base `load`, which already
prefetches the version. I checked the JDBC implementation registry: base and
ClickHouse are the only current `load` implementations.
I also covered the uncached getter fallback mentioned in your review
summary. During polish I found that a failed metadata read could still trigger
a nested fallback borrow; that now returns null for column parsing and retries
on the next load's held connection, with a counted regression test. All 538
non-Docker tests across the eight related modules pass.
##########
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:
Addressed in da228c71e3. The metadata proxy was already added in 2740544876,
but the actual `load` borrow-count coverage was still missing. The fixture now
accepts supplied metadata, and new tests run the real base
`load`/column-parsing path with null versions and a failed read followed by
recovery. They assert two parsed columns, peak borrowing of one, exactly one
borrow per load, and no connections left open.
The two real single-connection SQLite/DBCP reproductions remain. JDBC
common's connection-usage class now has 12 cases, including uncached getter
fallback and cache reset on reinitialization; the complete eight-module
non-Docker run passes 538 tests.
--
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]
