yuqi1129 commented on code in PR #13729:
URL: https://github.com/apache/gravitino/pull/13729#discussion_r4236822312
##########
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:
Fixed in 2740544876. A separate volatile `driverVersionCached` flag (written
after the version, so it publishes it) now records that the version was read,
so a null version is cached and `getMySQLDriverVersion()` no longer borrows
once `load` has read it from its held connection. A failed read (SQLException)
is still retried, since without the version MySQL datetime precision may be
computed from an inaccurate column size.
`testNullDriverVersionIsCachedFromTheHeldConnection` covers it and fails with
the old non-null check.
##########
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:
Same fix as the other thread: 2740544876 caches a null version through a
separate `driverVersionCached` flag.
##########
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:
Keeping this. The alter path sets no session state: each
`getConnection(databaseName)` sets the catalog on the borrowed connection, and
the statements carry everything else. Each DDL statement auto-commits, so the
old single connection did not make the sequence atomic either, and
partial-failure behavior is unchanged. Running generation's reads on the
execution connection would mean changing every generator's signature (they load
through `load`), while borrowing sequentially already bounds each request to
one connection at a time.
##########
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:
Keeping the name. It is a test fixture whose Javadoc describes it as a data
source whose connections only record SQL, and callers get the `DataSource`
through `dataSource()`; renaming would not change how the tests read.
##########
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:
See the other thread on the name: keeping it as is.
##########
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:
Keeping this. The handler only receives `equals` through a JDK proxy
invocation of `equals(Object)`, which always passes exactly one argument
(possibly null, which `proxy == args[0]` handles).
--
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]
