Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session property for jdbc metadata, pool size, caching enabled #23844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add session property for jdbc metadata, pool size, caching enabled #23844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package com.facebook.presto.plugin.jdbc;

import com.facebook.airlift.concurrent.BoundedExecutor;
import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.SchemaTableName;
Expand All @@ -27,9 +28,9 @@
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.concurrent.ExecutorService;

import static com.facebook.airlift.concurrent.Threads.daemonThreadsNamed;
import static com.facebook.presto.plugin.jdbc.JdbcMetadataSessionPropertiesProvider.isJdbcMetadataCacheEnabled;
import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Throwables.throwIfInstanceOf;
import static com.google.common.cache.CacheLoader.asyncReloading;
Expand All @@ -48,7 +49,7 @@ public class JdbcMetadataCache
public JdbcMetadataCache(JdbcClient jdbcClient, JdbcMetadataConfig config, JdbcMetadataCacheStats stats)
{
this(
newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")),
new BoundedExecutor(newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")), config.getMetadataCacheThreadPoolSize()),
jdbcClient,
stats,
OptionalLong.of(config.getMetadataCacheTtl().toMillis()),
Expand All @@ -57,15 +58,14 @@ public JdbcMetadataCache(JdbcClient jdbcClient, JdbcMetadataConfig config, JdbcM
}

public JdbcMetadataCache(
ExecutorService executor,
BoundedExecutor executor,
JdbcClient jdbcClient,
JdbcMetadataCacheStats stats,
OptionalLong cacheTtl,
OptionalLong refreshInterval,
long cacheMaximumSize)
{
this.jdbcClient = requireNonNull(jdbcClient, "jdbcClient is null");

this.tableHandleCache = newCacheBuilder(cacheTtl, refreshInterval, cacheMaximumSize)
.build(asyncReloading(CacheLoader.from(this::loadTableHandle), executor));
stats.setTableHandleCache(tableHandleCache);
Expand All @@ -77,11 +77,19 @@ public JdbcMetadataCache(

public JdbcTableHandle getTableHandle(ConnectorSession session, SchemaTableName tableName)
{
if (!isJdbcMetadataCacheEnabled(session)) {
return jdbcClient.getTableHandle(session, JdbcIdentity.from(session), tableName);
}

return get(tableHandleCache, new KeyAndSession<>(session, tableName)).orElse(null);
}

public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHandle jdbcTableHandle)
{
if (!isJdbcMetadataCacheEnabled(session)) {
return jdbcClient.getColumns(session, jdbcTableHandle);
}

return get(columnHandlesCache, new KeyAndSession<>(session, jdbcTableHandle));
}

Expand Down
27 changes: 27 additions & 0 deletions presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ public class JdbcMetadataConfig
private Duration metadataCacheTtl = new Duration(0, TimeUnit.SECONDS);
private Duration metadataCacheRefreshInterval = new Duration(0, TimeUnit.SECONDS);
private long metadataCacheMaximumSize = 10000;
private int metadataCacheThreadPoolSize = 10;
private boolean jdbcMetadataCacheEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also change the underlying feature flag methods and this to use names similar to useJdbcMetadataCache ?


public boolean isAllowDropTable()
{
Expand Down Expand Up @@ -83,4 +85,29 @@ public JdbcMetadataConfig setMetadataCacheMaximumSize(long metadataCacheMaximumS
this.metadataCacheMaximumSize = metadataCacheMaximumSize;
return this;
}

public int getMetadataCacheThreadPoolSize()
{
return metadataCacheThreadPoolSize;
}

@Min(1)
@Config("metadata-cache-thread-pool-size")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value represents the max background fetch threads for refreshing metadata, which IMO we should document.

@steveburnett I don't think we document these JdbcMetadataConfig properties at all.
Maybe we should rework the documentation for the different connectors that use these configs (like Postgres, MySQL) to have a common place for this and other JDBC related properties

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaneja thanks for the suggestion! Maybe a good place for documenting these features would be a new section in Presto Session Properties, titled JDBC Properties?

Let me know what you think, especially if you have a better idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. A dedicated section for common JDBC properties that we link to from other JDBC connectors would be ideal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aaneja!

Normally I would recommend that documentation for new configuration or session properties should be part of the PR. In this specific case that there is nowhere good to put them at this time, I've opened a new issue #23918 (doc PR to follow from that) to add a JDBC Properties topic to Presto Session Properties and populate it with these JdbcMetadataConfig properties to begin with. Other JDBC properties can be added to the new doc section in later PRs, and this code PR can move forward.

Sound reasonable?

public JdbcMetadataConfig setMetadataCacheThreadPoolSize(int threadPoolSize)
{
this.metadataCacheThreadPoolSize = threadPoolSize;
return this;
}

public boolean isJdbcMetadataCacheEnabled()
{
return jdbcMetadataCacheEnabled;
}

@Config("metadata-jdbc-cache-enabled")
public JdbcMetadataConfig setJdbcMetadataCacheEnabled(boolean jdbcMetadataCacheEnabled)
{
this.jdbcMetadataCacheEnabled = jdbcMetadataCacheEnabled;
return this;
}
}
58 changes: 58 additions & 0 deletions .../src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataSessionPropertiesProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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 com.facebook.presto.plugin.jdbc;

import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.session.PropertyMetadata;
import com.google.common.collect.ImmutableList;

import javax.inject.Inject;

import java.util.List;

import static com.facebook.presto.spi.session.PropertyMetadata.booleanProperty;

public class JdbcMetadataSessionPropertiesProvider
implements JdbcSessionPropertiesProvider
{
private final List<PropertyMetadata<?>> sessionProperties;
private static final String USE_JDBC_METADATA_CACHE = "use_jdbc_metadata_cache";

@Inject
public JdbcMetadataSessionPropertiesProvider(JdbcMetadataConfig jdbcMetadataConfig)
{
this.sessionProperties = ImmutableList.of(
booleanProperty(
USE_JDBC_METADATA_CACHE,
"Whether Jdbc Metadata In Memory Cache is Enabled",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make this more descriptive - Cache the result of the JDBC queries that fetch metadata about tables and columns

jdbcMetadataConfig.isJdbcMetadataCacheEnabled(),
false));
}

@Override
public List<PropertyMetadata<?>> getSessionProperties()
{
return sessionProperties;
}

public static boolean isJdbcMetadataCacheEnabled(ConnectorSession session)
{
try {
return session.getProperty(USE_JDBC_METADATA_CACHE, Boolean.class);
}
catch (Exception e) {
return false;
}
}
}
3 changes: 2 additions & 1 deletion presto-base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/TestJdbcMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package com.facebook.presto.plugin.jdbc;

import com.facebook.airlift.concurrent.BoundedExecutor;
import com.facebook.presto.spi.ColumnMetadata;
import com.facebook.presto.spi.ConnectorTableMetadata;
import com.facebook.presto.spi.PrestoException;
Expand Down Expand Up @@ -65,7 +66,7 @@ public void setUp()
throws Exception
{
database = new TestingDatabase();
ListeningExecutorService executor = listeningDecorator(newCachedThreadPool(daemonThreadsNamed("test-%s")));
BoundedExecutor executor = new BoundedExecutor(newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")), 1);
jdbcMetadataCache = new JdbcMetadataCache(executor, database.getJdbcClient(), new JdbcMetadataCacheStats(), OptionalLong.of(0), OptionalLong.of(0), 100);
metadata = new JdbcMetadata(jdbcMetadataCache, database.getJdbcClient(), false);
tableHandle = metadata.getTableHandle(SESSION, new SchemaTableName("example", "numbers"));
Expand Down
10 changes: 8 additions & 2 deletions presto-base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/TestJdbcMetadataConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ public void testDefaults()
.setAllowDropTable(false)
.setMetadataCacheTtl(new Duration(0, SECONDS))
.setMetadataCacheRefreshInterval(new Duration(0, SECONDS))
.setMetadataCacheMaximumSize(10000));
.setMetadataCacheMaximumSize(10000)
.setMetadataCacheThreadPoolSize(10)
.setJdbcMetadataCacheEnabled(false));
}

@Test
Expand All @@ -45,13 +47,17 @@ public void testExplicitPropertyMappings()
.put("metadata-cache-ttl", "1h")
.put("metadata-cache-refresh-interval", "10s")
.put("metadata-cache-maximum-size", "100")
.put("metadata-cache-thread-pool-size", "50")
.put("metadata-jdbc-cache-enabled", "true")
Copy link
Contributor

@shangm2 shangm2 Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to test another case where this config is not in the property and False should be expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is done in "testDefaults" test above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to test the branch where "session.getProperty(JDBC_METADATA_CACHE_ENABLED, Boolean.class)" throws an exception? Is there a way to generate the coverage report like JaCoCo?

.build();

JdbcMetadataConfig expected = new JdbcMetadataConfig()
.setAllowDropTable(true)
.setMetadataCacheTtl(new Duration(1, HOURS))
.setMetadataCacheRefreshInterval(new Duration(10, SECONDS))
.setMetadataCacheMaximumSize(100);
.setMetadataCacheMaximumSize(100)
.setMetadataCacheThreadPoolSize(50)
.setJdbcMetadataCacheEnabled(true);

assertFullMapping(properties, expected);
}
Expand Down
Loading