From 85d7f45821f303028ea0817149506abbeee55864 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Kokosi=C5=84ski?= Date: Sun, 6 Nov 2022 21:47:25 +0100 Subject: [PATCH] Extract QueryConfig from BaseJdbcConfig Conditional module requires to populate BaseJdbcConfig. Because of the fact some jdbc connectors overrides defaults of BaseJdbcConfig, these defaults are not visible during mentioned population and so that could lead to validation error of the config. Consider connectors that are not using connection-url config property. --- .../io/trino/plugin/jdbc/BaseJdbcConfig.java | 14 ------ .../java/io/trino/plugin/jdbc/JdbcModule.java | 4 +- .../io/trino/plugin/jdbc/QueryConfig.java | 35 ++++++++++++++ .../trino/plugin/jdbc/TestBaseJdbcConfig.java | 7 +-- .../io/trino/plugin/jdbc/TestQueryConfig.java | 46 +++++++++++++++++++ 5 files changed, 85 insertions(+), 21 deletions(-) create mode 100644 plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryConfig.java create mode 100644 plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestQueryConfig.java diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java index 953385df95bb..6dd954d24f2f 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcConfig.java @@ -44,7 +44,6 @@ public class BaseJdbcConfig private boolean cacheMissing; public static final long DEFAULT_METADATA_CACHE_SIZE = 10000; private long cacheMaximumSize = DEFAULT_METADATA_CACHE_SIZE; - private boolean reuseConnection = true; @NotNull // Some drivers match case insensitive in Driver.acceptURL @@ -115,19 +114,6 @@ public BaseJdbcConfig setCacheMaximumSize(long cacheMaximumSize) return this; } - public boolean isReuseConnection() - { - return reuseConnection; - } - - @Config("query.reuse-connection") - @ConfigDescription("Enables reusing JDBC connection for metadata queries to data source within a single Trino query") - public BaseJdbcConfig setReuseConnection(boolean reuseConnection) - { - this.reuseConnection = reuseConnection; - return this; - } - @PostConstruct public void validate() { 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 790f4f1af16b..4c1f6902465c 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 @@ -100,8 +100,8 @@ public void setup(Binder binder) .to(Key.get(ConnectionFactory.class, StatsCollecting.class)) .in(Scopes.SINGLETON); install(conditionalModule( - BaseJdbcConfig.class, - BaseJdbcConfig::isReuseConnection, + QueryConfig.class, + QueryConfig::isReuseConnection, new ReusableConnectionFactoryModule(), innerBinder -> innerBinder.bind(ConnectionFactory.class).to(LazyConnectionFactory.class).in(Scopes.SINGLETON))); diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryConfig.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryConfig.java new file mode 100644 index 000000000000..a24dbda435c8 --- /dev/null +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryConfig.java @@ -0,0 +1,35 @@ +/* + * 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 io.airlift.configuration.Config; +import io.airlift.configuration.ConfigDescription; + +public class QueryConfig +{ + private boolean reuseConnection = true; + + public boolean isReuseConnection() + { + return reuseConnection; + } + + @Config("query.reuse-connection") + @ConfigDescription("Enables reusing JDBC connection for metadata queries to data source within a single Trino query") + public QueryConfig setReuseConnection(boolean reuseConnection) + { + this.reuseConnection = reuseConnection; + return this; + } +} diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestBaseJdbcConfig.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestBaseJdbcConfig.java index bc8a6b40dd26..c78ca674cc0d 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestBaseJdbcConfig.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestBaseJdbcConfig.java @@ -41,8 +41,7 @@ public void testDefaults() .setJdbcTypesMappedToVarchar("") .setMetadataCacheTtl(ZERO) .setCacheMissing(false) - .setCacheMaximumSize(10000) - .setReuseConnection(true)); + .setCacheMaximumSize(10000)); } @Test @@ -54,7 +53,6 @@ public void testExplicitPropertyMappings() .put("metadata.cache-ttl", "1s") .put("metadata.cache-missing", "true") .put("metadata.cache-maximum-size", "5000") - .put("query.reuse-connection", "false") .buildOrThrow(); BaseJdbcConfig expected = new BaseJdbcConfig() @@ -62,8 +60,7 @@ public void testExplicitPropertyMappings() .setJdbcTypesMappedToVarchar("mytype, struct_type1") .setMetadataCacheTtl(new Duration(1, SECONDS)) .setCacheMissing(true) - .setCacheMaximumSize(5000) - .setReuseConnection(false); + .setCacheMaximumSize(5000); assertFullMapping(properties, expected); diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestQueryConfig.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestQueryConfig.java new file mode 100644 index 000000000000..4ce587551f9e --- /dev/null +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestQueryConfig.java @@ -0,0 +1,46 @@ +/* + * 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.common.collect.ImmutableMap; +import org.testng.annotations.Test; + +import java.util.Map; + +import static io.airlift.configuration.testing.ConfigAssertions.assertFullMapping; +import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults; +import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; + +public class TestQueryConfig +{ + @Test + public void testDefaults() + { + assertRecordedDefaults(recordDefaults(QueryConfig.class) + .setReuseConnection(true)); + } + + @Test + public void testExplicitPropertyMappings() + { + Map properties = ImmutableMap.builder() + .put("query.reuse-connection", "false") + .buildOrThrow(); + + QueryConfig expected = new QueryConfig() + .setReuseConnection(false); + + assertFullMapping(properties, expected); + } +}