Copilot commented on code in PR #13729:
URL: https://github.com/apache/gravitino/pull/13729#discussion_r4231372356
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -754,21 +805,44 @@ 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() {
+ String version = driverVersion;
+ if (version != null || dataSource == null) {
+ return version;
+ }
+ try (Connection connection = dataSource.getConnection()) {
+ cacheDriverVersion(connection);
+ } catch (SQLException e) {
+ LOG.debug("Failed to get driver version", e);
+ }
+ return driverVersion;
+ }
Review Comment:
`cacheDriverVersion()` only short-circuits when `driverVersion != null`. If
`DatabaseMetaData.getDriverVersion()` returns `null` (allowed per the method’s
contract), `driverVersion` remains `null` and subsequent calls to
`getMySQLDriverVersion()` will keep borrowing new connections. In the worst
case, this reintroduces the exact nested-borrow problem during `load()` (column
parsing calls `getMySQLDriverVersion()` while a connection is held),
potentially causing pool starvation again. Fix by tracking “attempted/fetched”
separately from the string value (e.g., a `volatile boolean
driverVersionFetched`, or storing a sentinel like empty string and translating
it back to `null` on return), and ensuring `getMySQLDriverVersion()` never
borrows again once a fetch attempt has been made.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -754,21 +805,44 @@ 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() {
+ String version = driverVersion;
+ if (version != null || dataSource == null) {
+ return version;
+ }
+ 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
+ */
+ protected void cacheDriverVersion(Connection connection) {
+ if (driverVersion != null) {
+ return;
+ }
try {
- if (dataSource != null) {
- try (Connection connection = dataSource.getConnection()) {
- return connection.getMetaData().getDriverVersion();
- }
- }
+ driverVersion = connection.getMetaData().getDriverVersion();
} catch (SQLException e) {
LOG.debug("Failed to get driver version", e);
}
- return null;
}
Review Comment:
`cacheDriverVersion()` only short-circuits when `driverVersion != null`. If
`DatabaseMetaData.getDriverVersion()` returns `null` (allowed per the method’s
contract), `driverVersion` remains `null` and subsequent calls to
`getMySQLDriverVersion()` will keep borrowing new connections. In the worst
case, this reintroduces the exact nested-borrow problem during `load()` (column
parsing calls `getMySQLDriverVersion()` while a connection is held),
potentially causing pool starvation again. Fix by tracking “attempted/fetched”
separately from the string value (e.g., a `volatile boolean
driverVersionFetched`, or storing a sentinel like empty string and translating
it back to `null` on return), and ensuring `getMySQLDriverVersion()` never
borrows again once a fetch attempt has been made.
##########
catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/ConnectionCountingDataSource.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.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 {
Review Comment:
The type is named `ConnectionCountingDataSource` but it is not itself a
`DataSource` (it’s a wrapper that exposes one via `dataSource()`). In tests
this reads slightly misleadingly (e.g., passing it around as if it were a
`DataSource`). Consider either (a) implementing `DataSource` directly and
delegating to the proxy, or (b) renaming to something like
`ConnectionCountingDataSourceWrapper`/`ConnectionCountingDataSourceFixture` to
reflect its role.
##########
catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/ConnectionCountingDataSource.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.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 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;
+ }
Review Comment:
The type is named `ConnectionCountingDataSource` but it is not itself a
`DataSource` (it’s a wrapper that exposes one via `dataSource()`). In tests
this reads slightly misleadingly (e.g., passing it around as if it were a
`DataSource`). Consider either (a) implementing `DataSource` directly and
delegating to the proxy, or (b) renaming to something like
`ConnectionCountingDataSourceWrapper`/`ConnectionCountingDataSourceFixture` to
reflect its role.
##########
catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java:
##########
@@ -211,19 +211,22 @@ protected String generatePurgeTableSql(String tableName) {
public void alterTable(String databaseName, String tableName, TableChange...
changes)
throws NoSuchTableException {
LOG.info("Attempting to alter table {} from database {}", tableName,
databaseName);
- try (Connection connection = getConnection(databaseName)) {
- for (TableChange change : changes) {
- String sql = generateAlterTableSql(databaseName, tableName, change);
- if (StringUtils.isEmpty(sql)) {
- LOG.info("No changes to alter table {} from database {}", tableName,
databaseName);
- continue;
- }
+ for (TableChange change : changes) {
+ // Generate each statement before borrowing the connection that executes
it, as the
+ // generator may load the table through another connection. Each change
is generated after
+ // the previous one is applied, so it sees the updated table.
+ String sql = generateAlterTableSql(databaseName, tableName, change);
+ if (StringUtils.isEmpty(sql)) {
+ LOG.info("No changes to alter table {} from database {}", tableName,
databaseName);
+ continue;
+ }
+ try (Connection connection = getConnection(databaseName)) {
JdbcConnectorUtils.executeUpdate(connection, sql);
+ } catch (final SQLException se) {
+ throw this.exceptionMapper.toGravitinoException(se);
}
Review Comment:
This change deliberately borrows a new pooled connection per non-empty
statement. While it fixes nested-borrow starvation, it may increase pool churn
and can change session-level semantics if OceanBase relies on
connection/session state across sequential statements (e.g., session variables,
temporary settings). If session continuity is important, consider an
alternative design where SQL generation is refactored to avoid borrowing a
second connection (e.g., allow generators to use the same execution connection
for any required reads), so the whole alter can run on one connection without
nested borrows.
##########
catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/ConnectionCountingDataSource.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.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 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 the update statements executed so far, in order. */
+ public List<String> executedSql() {
+ return executedSql;
+ }
+
+ private Connection borrow() {
+ 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();
+ }
+ return null;
+ case "setCatalog":
+ case "setSchema":
+ return null;
+ case "createStatement":
+ return statement();
+ default:
+ return objectMethod(proxy, method.getName(), args,
"Connection");
+ }
+ });
+ }
+
+ // Answers Object methods so the proxies can be printed or compared, and
rejects anything else.
+ private static Object objectMethod(Object proxy, String name, Object[] args,
String type) {
+ switch (name) {
+ case "toString":
+ return "Counting" + type + "@" +
Integer.toHexString(System.identityHashCode(proxy));
+ case "hashCode":
+ return System.identityHashCode(proxy);
+ case "equals":
+ return proxy == args[0];
+ default:
+ throw new UnsupportedOperationException(type + "." + name);
+ }
+ }
Review Comment:
`equals` assumes `args` is non-null and has at least one element. While
`equals(Object)` should typically meet that, defensive handling here would make
the test utility more robust and easier to reuse (e.g., handle `args == null ||
args.length != 1` by returning `false`). This avoids surprising
`NullPointerException` if the handler is ever invoked in an unexpected way.
--
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]
