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

konjac-h · 2024-10-16T11:52:10Z

Description

This pull request enables the control of JDBC in-memory metadata caching through the use of a session property. Additionally, it also provides the capability to manage the thread pool size.

Motivation and Context

Currently, Presto does not have the capability to control the JDBC metadata thread pool size and caching for certain sources. This pull request aims to address this limitation by making these features available in Presto.

Impact

as above

Test Plan

build & deploy to verifier that the change will be picked up by inspecting session property at query level

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

aaneja · 2024-10-16T12:42:03Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataCache.java

@@ -48,7 +49,7 @@ public class JdbcMetadataCache
    public JdbcMetadataCache(JdbcClient jdbcClient, JdbcMetadataConfig config, JdbcMetadataCacheStats stats)
    {
        this(
-                newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")),
+                newFixedThreadPool(config.getMetadataCacheThreadPoolSize(), daemonThreadsNamed("jdbc-metadata-cache" + "-%s")),


The thread pool created with the newCachedThreadPool will spin up threads on demand for the asyncReloading to occur. Why is a fixed size thread pool better here ?

I think he is trying to limit the numbers of threads that can used for the refresh.

The following is better. its still a cache thread pool, but we can limit it
new BoundedExecutor(newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")), config.getMetadataCacheThreadPoolSize()),

aaneja · 2024-10-16T12:48:13Z

...dbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataSessionPropertiesProvider.java

+        implements JdbcSessionPropertiesProvider
+{
+    private final List<PropertyMetadata<?>> sessionProperties;
+    private static final String JDBC_METADATA_CACHE_ENABLED = "jdbc_metadata_cache_enabled";


Since the cache is always created, maybe this session property is better named - use_jdbc_metadata_cache ?

sounds good. thanks

steveburnett · 2024-10-16T14:18:32Z

Consider adding documentation for this new session property.

shangm2 · 2024-10-16T19:56:20Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataCache.java

@@ -48,7 +49,7 @@ public class JdbcMetadataCache
    public JdbcMetadataCache(JdbcClient jdbcClient, JdbcMetadataConfig config, JdbcMetadataCacheStats stats)
    {
        this(
-                newCachedThreadPool(daemonThreadsNamed("jdbc-metadata-cache" + "-%s")),
+                newFixedThreadPool(config.getMetadataCacheThreadPoolSize(), daemonThreadsNamed("jdbc-metadata-cache" + "-%s")),


shangm2 · 2024-10-16T19:57:08Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataConfig.java

+        return this;
+    }
+
+    public boolean getJdbcMetadataCacheEnabled()


nit: isJdbcMetadataCacheEnabled()

shangm2 · 2024-10-16T19:59:55Z

presto-base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/TestJdbcMetadataConfig.java

@@ -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")


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

that is done in "testDefaults" test above

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?

aaneja · 2024-10-29T13:16:18Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataConfig.java

@@ -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;


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

aaneja · 2024-10-29T13:22:22Z

...dbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataSessionPropertiesProvider.java

+        this.sessionProperties = ImmutableList.of(
+                booleanProperty(
+                        USE_JDBC_METADATA_CACHE,
+                        "Whether Jdbc Metadata In Memory Cache is Enabled",


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

aaneja · 2024-10-29T13:36:54Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcMetadataConfig.java

+    }
+
+    @Min(1)
+    @Config("metadata-cache-thread-pool-size")


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

@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.

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

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?

konjac-h requested a review from a team as a code owner October 16, 2024 11:52

konjac-h requested a review from presto-oss October 16, 2024 11:52

konjac-h force-pushed the add-jdbc-metadata-session-properties branch from 8d0682c to 28ee5b2 Compare October 16, 2024 12:08

aaneja reviewed Oct 16, 2024

View reviewed changes

tdcmeehan self-assigned this Oct 16, 2024

shangm2 reviewed Oct 16, 2024

View reviewed changes

konjac-h force-pushed the add-jdbc-metadata-session-properties branch from 28ee5b2 to 7101209 Compare October 24, 2024 18:48

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

15fdc33

konjac-h force-pushed the add-jdbc-metadata-session-properties branch from 7101209 to 15fdc33 Compare October 24, 2024 18:54

aaneja reviewed Oct 29, 2024

View reviewed changes

steveburnett mentioned this pull request Oct 30, 2024

[docs] Add JDBC Properties section to admin/properties-session.rst #23918

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

konjac-h commented Oct 16, 2024

aaneja Oct 16, 2024

NikhilCollooru Oct 16, 2024 •

edited

Loading

shangm2 Oct 16, 2024

aaneja Oct 16, 2024

konjac-h Oct 16, 2024

steveburnett commented Oct 16, 2024

shangm2 Oct 16, 2024

shangm2 Oct 16, 2024

shangm2 Oct 16, 2024 •

edited

Loading

NikhilCollooru Oct 16, 2024

shangm2 Oct 17, 2024

aaneja Oct 29, 2024

aaneja Oct 29, 2024

aaneja Oct 29, 2024

steveburnett Oct 29, 2024

aaneja Oct 30, 2024

steveburnett Oct 30, 2024

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

Are you sure you want to change the base?

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

Conversation

konjac-h commented Oct 16, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

NikhilCollooru Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shangm2 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikhilCollooru Oct 16, 2024 •

edited

Loading

shangm2 Oct 16, 2024 •

edited

Loading