From 1206ec636c391523f4d04e648401cf7f64ea3021 Mon Sep 17 00:00:00 2001 From: Dominik Zalewski Date: Tue, 10 Oct 2023 16:49:30 +0200 Subject: [PATCH] Simplify ConnectionFactory chain of responsibility --- .../plugin/jdbc/ForLazyConnectionFactory.java | 31 ------------------- .../plugin/jdbc/JdbcDiagnosticModule.java | 12 ++----- .../java/io/trino/plugin/jdbc/JdbcModule.java | 4 --- .../plugin/jdbc/LazyConnectionFactory.java | 5 +-- .../jdbc/RetryingConnectionFactory.java | 5 +-- .../jmx/StatisticsAwareConnectionFactory.java | 5 ++- .../jdbc/TestLazyConnectionFactory.java | 6 ++-- .../jdbc/TestRetryingConnectionFactory.java | 2 +- .../plugin/oracle/OracleClientModule.java | 5 +-- .../plugin/oracle/TestingOracleServer.java | 5 +-- .../plugin/phoenix5/PhoenixClientModule.java | 5 --- 11 files changed, 23 insertions(+), 62 deletions(-) delete mode 100644 plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java deleted file mode 100644 index 21ee99114bd5..000000000000 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForLazyConnectionFactory.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Licensed 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 io.trino.plugin.jdbc; - -import com.google.inject.BindingAnnotation; - -import java.lang.annotation.Retention; -import java.lang.annotation.Target; - -import static java.lang.annotation.ElementType.FIELD; -import static java.lang.annotation.ElementType.METHOD; -import static java.lang.annotation.ElementType.PARAMETER; -import static java.lang.annotation.RetentionPolicy.RUNTIME; - -@Retention(RUNTIME) -@Target({FIELD, PARAMETER, METHOD}) -@BindingAnnotation -public @interface ForLazyConnectionFactory -{ -} diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDiagnosticModule.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDiagnosticModule.java index 557095b85053..93bed3852ec8 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDiagnosticModule.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDiagnosticModule.java @@ -18,6 +18,7 @@ import com.google.inject.Module; import com.google.inject.Provider; import com.google.inject.Provides; +import com.google.inject.Scopes; import com.google.inject.Singleton; import io.airlift.log.Logger; import io.trino.plugin.base.CatalogName; @@ -39,11 +40,12 @@ public void configure(Binder binder) { binder.install(new MBeanServerModule()); binder.install(new MBeanModule()); + binder.bind(StatisticsAwareConnectionFactory.class).in(Scopes.SINGLETON); Provider catalogName = binder.getProvider(CatalogName.class); newExporter(binder).export(Key.get(JdbcClient.class, StatsCollecting.class)) .as(generator -> generator.generatedNameOf(JdbcClient.class, catalogName.get().toString())); - newExporter(binder).export(Key.get(ConnectionFactory.class, StatsCollecting.class)) + newExporter(binder).export(StatisticsAwareConnectionFactory.class) .as(generator -> generator.generatedNameOf(ConnectionFactory.class, catalogName.get().toString())); newExporter(binder).export(JdbcClient.class) .as(generator -> generator.generatedNameOf(CachingJdbcClient.class, catalogName.get().toString())); @@ -65,12 +67,4 @@ public JdbcClient createJdbcClientWithStats(@ForBaseJdbc JdbcClient client, Cata return client; })); } - - @Provides - @Singleton - @StatsCollecting - public static ConnectionFactory createConnectionFactoryWithStats(@ForBaseJdbc ConnectionFactory connectionFactory) - { - return new StatisticsAwareConnectionFactory(connectionFactory); - } } diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java index fbb3d98760aa..f66b5242d888 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java @@ -88,10 +88,6 @@ public void setup(Binder binder) newSetBinder(binder, ConnectorTableFunction.class); - binder.bind(ConnectionFactory.class) - .annotatedWith(ForLazyConnectionFactory.class) - .to(Key.get(ConnectionFactory.class, StatsCollecting.class)) - .in(Scopes.SINGLETON); install(conditionalModule( QueryConfig.class, QueryConfig::isReuseConnection, diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java index f056e07c7b95..9db72ad77fa5 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java @@ -16,6 +16,7 @@ import com.google.errorprone.annotations.ThreadSafe; import com.google.errorprone.annotations.concurrent.GuardedBy; import com.google.inject.Inject; +import io.trino.plugin.jdbc.jmx.StatisticsAwareConnectionFactory; import io.trino.spi.connector.ConnectorSession; import jakarta.annotation.Nullable; @@ -29,10 +30,10 @@ public final class LazyConnectionFactory implements ConnectionFactory { - private final ConnectionFactory delegate; + private final StatisticsAwareConnectionFactory delegate; @Inject - public LazyConnectionFactory(@ForLazyConnectionFactory ConnectionFactory delegate) + public LazyConnectionFactory(StatisticsAwareConnectionFactory delegate) { this.delegate = requireNonNull(delegate, "delegate is null"); } diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java index fe491ffcb977..38cb7bdf1b4a 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/RetryingConnectionFactory.java @@ -18,6 +18,7 @@ import dev.failsafe.Failsafe; import dev.failsafe.FailsafeException; import dev.failsafe.RetryPolicy; +import io.trino.plugin.jdbc.jmx.StatisticsAwareConnectionFactory; import io.trino.spi.TrinoException; import io.trino.spi.connector.ConnectorSession; @@ -40,10 +41,10 @@ public class RetryingConnectionFactory .abortOn(TrinoException.class) .build(); - private final ConnectionFactory delegate; + private final StatisticsAwareConnectionFactory delegate; @Inject - public RetryingConnectionFactory(@StatsCollecting ConnectionFactory delegate) + public RetryingConnectionFactory(StatisticsAwareConnectionFactory delegate) { this.delegate = requireNonNull(delegate, "delegate is null"); } diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareConnectionFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareConnectionFactory.java index 6c9153997a06..9f59238ba790 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareConnectionFactory.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareConnectionFactory.java @@ -13,7 +13,9 @@ */ package io.trino.plugin.jdbc.jmx; +import com.google.inject.Inject; import io.trino.plugin.jdbc.ConnectionFactory; +import io.trino.plugin.jdbc.ForBaseJdbc; import io.trino.spi.connector.ConnectorSession; import org.weakref.jmx.Managed; import org.weakref.jmx.Nested; @@ -30,7 +32,8 @@ public class StatisticsAwareConnectionFactory private final JdbcApiStats closeConnection = new JdbcApiStats(); private final ConnectionFactory delegate; - public StatisticsAwareConnectionFactory(ConnectionFactory delegate) + @Inject + public StatisticsAwareConnectionFactory(@ForBaseJdbc ConnectionFactory delegate) { this.delegate = requireNonNull(delegate, "delegate is null"); } diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java index e23f34fc3349..a6b41a1f9f07 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestLazyConnectionFactory.java @@ -32,7 +32,7 @@ public class TestLazyConnectionFactory public void testNoConnectionIsCreated() throws Exception { - Injector injector = Guice.createInjector(binder -> binder.bind(ConnectionFactory.class).annotatedWith(ForLazyConnectionFactory.class).toInstance( + Injector injector = Guice.createInjector(binder -> binder.bind(ConnectionFactory.class).annotatedWith(ForBaseJdbc.class).toInstance( session -> { throw new AssertionError("Expected no connection creation"); })); @@ -50,8 +50,8 @@ public void testConnectionCannotBeReusedAfterClose() BaseJdbcConfig config = new BaseJdbcConfig() .setConnectionUrl(format("jdbc:h2:mem:test%s;DB_CLOSE_DELAY=-1", System.nanoTime() + ThreadLocalRandom.current().nextLong())); - Injector injector = Guice.createInjector(binder -> binder.bind(ConnectionFactory.class).annotatedWith(ForLazyConnectionFactory.class).toInstance( - new DriverConnectionFactory(new Driver(), config, new EmptyCredentialProvider()))); + Injector injector = Guice.createInjector(binder -> binder.bind(ConnectionFactory.class).annotatedWith(ForBaseJdbc.class).toInstance( + new DriverConnectionFactory(new Driver(), config, new EmptyCredentialProvider()))); try (LazyConnectionFactory lazyConnectionFactory = injector.getInstance(LazyConnectionFactory.class)) { Connection connection = lazyConnectionFactory.openConnection(SESSION); diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java index 908d062b8e4c..eda556b7be9d 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestRetryingConnectionFactory.java @@ -140,7 +140,7 @@ private static Injector createInjector(MockConnectorFactory.Action... actions) return Guice.createInjector(binder -> { binder.bind(MockConnectorFactory.Action[].class).toInstance(actions); binder.bind(MockConnectorFactory.class).in(Scopes.SINGLETON); - binder.bind(ConnectionFactory.class).annotatedWith(StatsCollecting.class).to(Key.get(MockConnectorFactory.class)); + binder.bind(ConnectionFactory.class).annotatedWith(ForBaseJdbc.class).to(Key.get(MockConnectorFactory.class)); }); } diff --git a/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java b/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java index af36cb507e88..891844ed1d51 100644 --- a/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java +++ b/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClientModule.java @@ -28,6 +28,7 @@ import io.trino.plugin.jdbc.MaxDomainCompactionThreshold; import io.trino.plugin.jdbc.RetryingConnectionFactory; import io.trino.plugin.jdbc.credential.CredentialProvider; +import io.trino.plugin.jdbc.jmx.StatisticsAwareConnectionFactory; import io.trino.plugin.jdbc.ptf.Query; import io.trino.spi.function.table.ConnectorTableFunction; import oracle.jdbc.OracleConnection; @@ -76,11 +77,11 @@ public static ConnectionFactory connectionFactory(BaseJdbcConfig config, Credent openTelemetry); } - return new RetryingConnectionFactory(new DriverConnectionFactory( + return new RetryingConnectionFactory(new StatisticsAwareConnectionFactory(new DriverConnectionFactory( new OracleDriver(), config.getConnectionUrl(), connectionProperties, credentialProvider, - openTelemetry)); + openTelemetry))); } } diff --git a/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestingOracleServer.java b/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestingOracleServer.java index f73ae90215f3..976d80e5e83e 100644 --- a/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestingOracleServer.java +++ b/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestingOracleServer.java @@ -23,6 +23,7 @@ import io.trino.plugin.jdbc.DriverConnectionFactory; import io.trino.plugin.jdbc.RetryingConnectionFactory; import io.trino.plugin.jdbc.credential.StaticCredentialProvider; +import io.trino.plugin.jdbc.jmx.StatisticsAwareConnectionFactory; import io.trino.testing.ResourcePresence; import oracle.jdbc.OracleDriver; import org.testcontainers.containers.OracleContainer; @@ -125,10 +126,10 @@ public void execute(String sql, String user, String password) private ConnectionFactory getConnectionFactory(String connectionUrl, String username, String password) { - DriverConnectionFactory connectionFactory = new DriverConnectionFactory( + StatisticsAwareConnectionFactory connectionFactory = new StatisticsAwareConnectionFactory(new DriverConnectionFactory( new OracleDriver(), new BaseJdbcConfig().setConnectionUrl(connectionUrl), - StaticCredentialProvider.of(username, password)); + StaticCredentialProvider.of(username, password))); return new RetryingConnectionFactory(connectionFactory); } diff --git a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java index 852d76723aa4..3a617b6ede59 100644 --- a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java +++ b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java @@ -34,7 +34,6 @@ import io.trino.plugin.jdbc.DynamicFilteringStats; import io.trino.plugin.jdbc.ForBaseJdbc; import io.trino.plugin.jdbc.ForJdbcDynamicFiltering; -import io.trino.plugin.jdbc.ForLazyConnectionFactory; import io.trino.plugin.jdbc.ForRecordCursor; import io.trino.plugin.jdbc.JdbcClient; import io.trino.plugin.jdbc.JdbcDiagnosticModule; @@ -130,10 +129,6 @@ protected void setup(Binder binder) binder.bind(ConnectorMetadata.class).annotatedWith(ForClassLoaderSafe.class).to(PhoenixMetadata.class).in(Scopes.SINGLETON); binder.bind(ConnectorMetadata.class).to(ClassLoaderSafeConnectorMetadata.class).in(Scopes.SINGLETON); - binder.bind(ConnectionFactory.class) - .annotatedWith(ForLazyConnectionFactory.class) - .to(Key.get(ConnectionFactory.class, StatsCollecting.class)) - .in(Scopes.SINGLETON); install(conditionalModule( PhoenixConfig.class, PhoenixConfig::isReuseConnection,