Add a DB based session property manager by infvg · Pull Request #24995 · prestodb/presto

infvg · 2025-04-28T11:34:34Z

Description

Add a DB based session property manager.

Motivation and Context

Will add the option of adding a new session property manager

Impact

A new db-based session property manager. Will be off by default but can be turned on using the config session-property-config.configuration-manager=db

Test Plan

Unit tests

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
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

== RELEASE NOTES ==

General Changes
* Added a new db-based session property manager. :doc:`/admin/session-property-managers`

pratyakshsharma

Thank you for raising this PR @infvg. One high level comment - please update the docs section for "Session Property Managers" with file and db based managers that will be supported post this change.
I am still going over the changes.

steveburnett

Thanks for the documentation! A few suggestions.

steveburnett

Minor phrasing suggestion.

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

imjalpreet

@infvg, thank you, I finished my first pass, please have a look.

imjalpreet · 2025-07-15T07:29:56Z

+    session-property-manager.db.url=url
+    session-property-manager.db.username=username
+    session-property-manager.db.password=password
+    session-property-manager.db.refresh-period=50s


This is missing one config. Please add session-property-config.configuration-manager=db as that is required to enable the DB-based session property manager.

@infvg I think we should also add a brief description of every new property introduced. @steveburnett, do you think that would be useful?

@imjalpreet yes, I agree. All properties should be documented. See Designing Your Code section iii in CONTRIBUTING.md.

Added an explanation for session-property-manager.db.url and session-property-manager.db.refresh-period. I also added the session-property-config.configuration-manager=db

imjalpreet · 2025-07-15T07:31:06Z

+
+.. code-block:: text
+
+    presto:tpch> DESCRIBE session_specs;


This table is present in the MySQL DB. The CLI prompt presto:tpch> is a little confusing.

Maybe we should show output from MySQL CLI rather than Presto CLI.

imjalpreet · 2025-07-15T07:31:57Z

+
+.. code-block:: text
+
+    presto:tpch> DESCRIBE session_client_tags;


This table is present in the MySQL DB. The CLI prompt presto:tpch> is a little confusing.

imjalpreet · 2025-07-15T07:32:07Z

+
+.. code-block:: text
+
+    presto:tpch> DESCRIBE session_property_values;


This table is present in the MySQL DB. The CLI prompt presto:tpch> is a little confusing.

imjalpreet · 2025-07-15T07:39:08Z

+    session-property-manager.db.password=password
+    session-property-manager.db.refresh-period=50s
+
+This database consists of three tables: session specs, client tags and properties.


We should also note in the documentation that these tables will be automatically created in the configured database on the first server startup after the configurations are added, if they do not already exist.

Added a note

imjalpreet · 2025-07-15T08:04:21Z

+import static com.facebook.presto.session.file.FileSessionPropertyManager.CODEC;
+import static org.testng.Assert.assertEquals;
+
+public class TestFileSessionPropertyManager


Can we add a similar test for the DB-based session property manager?

Added tests

imjalpreet · 2025-07-15T08:10:32Z

        return resourceGroupManager;
    }

+    public SessionPropertyDefaults getSessionPropertyDefaults()


Do we need the changes in this file? I don't see any usage.

imjalpreet · 2025-07-15T08:12:23Z

+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>testing-mysql-server</artifactId>
+            <version>8.0.12-2</version>
+            <exclusions>
+                <exclusion>
+                    <groupId>mysql</groupId>
+                    <artifactId>mysql-connector-java</artifactId>
+                </exclusion>
+            </exclusions>
+            <scope>test</scope>
+        </dependency>


I don't see any tests added yet that are using this dependency.

Changed to similar mysql dependencies that are used in the new tests.

imjalpreet · 2025-07-15T08:40:58Z

+    @Override
+    public SystemSessionPropertyConfiguration getSystemSessionProperties(SessionConfigurationContext context)
+    {
+        List<SessionMatchSpec> sessionMatchSpecs = specsProvider.get();
+
+        // later properties override earlier properties
+        Map<String, String> defaultProperties = new HashMap<>();
+        Set<String> overridePropertyNames = new HashSet<String>();
+        for (SessionMatchSpec sessionMatchSpec : sessionMatchSpecs) {
+            Map<String, String> newProperties = sessionMatchSpec.match(context);
+            defaultProperties.putAll(newProperties);
+            if (sessionMatchSpec.getOverrideSessionProperties().orElse(false)) {
+                overridePropertyNames.addAll(newProperties.keySet());
+            }
+        }
+
+        // Once a property has been overridden it stays that way and the value is updated by any rule
+        Map<String, String> overrideProperties = new HashMap<>();
+        for (String propertyName : overridePropertyNames) {
+            overrideProperties.put(propertyName, defaultProperties.get(propertyName));
+        }
+
+        return new SystemSessionPropertyConfiguration(ImmutableMap.copyOf(defaultProperties), ImmutableMap.copyOf(overrideProperties));
+    }
+
+    @Override
+    public Map<String, Map<String, String>> getCatalogSessionProperties(SessionConfigurationContext context)
+    {
+        // NOT IMPLEMENTED YET
+        return ImmutableMap.of();
+    }
+}


We shouldn't override these. Based on what I understand, we can just override the abstract method: getSessionMatchSpecs in all implementations of AbstractSessionPropertyManager

imjalpreet

@infvg, please rebase on the latest master as well to trigger all tests.

imjalpreet

@infvg, thank you! Mostly LGTM, just a couple of suggestions.

imjalpreet · 2025-08-26T09:36:10Z

It might be useful for Presto users if we add a note in this doc that it is recommended to use DB-based session property managers in production, as the properties can be updated without requiring a cluster restart, which is not possible in the case of file file-based session property manager.

Added to the docs

imjalpreet · 2025-08-26T09:38:22Z

nit: please revert the whitespace-only changes

imjalpreet · 2025-08-26T09:38:25Z

nit: please revert the whitespace-only changes

imjalpreet · 2025-08-26T09:46:19Z

@tdcmeehan, do we want to split this module?

libianoss · 2025-08-26T09:59:17Z

@infvg @tdcmeehan pls help to check the comments from @imjalpreet and let's try to complete and merge this PR by this Thur. Thank you.

imjalpreet

@infvg, thank you, mostly looks good. Can you please add details regarding the refactor in the commit description as well?

imjalpreet · 2025-08-28T21:03:29Z

            <artifactId>jakarta.validation-api</artifactId>
        </dependency>

+


nit: remove extra line

imjalpreet · 2025-08-28T21:04:11Z

            <groupId>com.fasterxml.jackson.core</groupId>
            <artifactId>jackson-core</artifactId>
        </dependency>
-


nit: keep this new line

imjalpreet · 2025-08-28T22:13:22Z

Is it possible to keep a separate commit for this, or does it start erroring out if it's not updated in the same commit?

Split into 2 commits

libianoss · 2025-09-01T08:58:40Z

@infvg can I ask what's pending for this PR now? @imjalpreet are all things working fine now?

infvg · 2025-09-01T09:00:02Z

@libianoss PR is ready for review

imjalpreet

Thank you, @infvg. LGTM % last few nits.

imjalpreet · 2025-09-01T09:37:16Z

+        <dependency>
+            <groupId>com.mysql</groupId>
+            <artifactId>mysql-connector-j</artifactId>
+            <optional>true</optional>
+            <scope>runtime</scope>
+        </dependency>
+
+        <dependency>
+            <groupId>org.mariadb.jdbc</groupId>
+            <artifactId>mariadb-java-client</artifactId>
+            <optional>true</optional>
+            <scope>runtime</scope>
+        </dependency>
+


We shouldn't need these in this module, right?

Yep, removed

imjalpreet · 2025-09-01T09:38:15Z

nit: please remove these whitespace-only changes

imjalpreet · 2025-09-01T09:38:20Z

nit: please remove these whitespace-only changes

Co-authored-by: Ashish Tadose <ashishtadose@gmail.com> Co-authored-by: Ariel Weisberg <aweisberg@fb.com> Co-authored-by: Jalpreet Singh Nanda (:imjalpreet) <jalpreetnanda@gmail.com>

Currently, the presto-session-property-managers module has gotten large due to the different implementations for file based and db based session property managers. If implemented, this commit will split the module into 3, ``presto-session-property-managers-common`` will contain the API implementation for a session property manager along with some tests. ``presto-file-session-property-manager`` will contain the file based implementation, and ``presto-db-session-property-manager`` will contain the db based implementation.

imjalpreet

Thank you, @infvg.

libianoss · 2025-09-01T15:07:25Z

@imjalpreet jal @infvg is this ready to merge?

libianoss · 2025-09-03T09:03:44Z

@infvg can we get this merged? What's the blocker now?

libianoss · 2025-09-03T15:23:07Z

@infvg can you response to the request from @pratyakshsharma ?

prestodb-ci added the from:IBM PR from IBM label Apr 28, 2025

infvg force-pushed the db-session-property branch from b9191b4 to a405418 Compare April 28, 2025 11:37

infvg force-pushed the db-session-property branch 2 times, most recently from 064f35e to 9f7283f Compare May 25, 2025 08:30

infvg marked this pull request as ready for review May 25, 2025 10:25

infvg requested review from a team and shrinidhijoshi as code owners May 25, 2025 10:25

infvg requested a review from hantangwangd May 25, 2025 10:25

prestodb-ci requested review from a team, ShahimSharafudeen and wanglinsong and removed request for a team May 25, 2025 10:25

infvg force-pushed the db-session-property branch 2 times, most recently from c335922 to b29057b Compare May 26, 2025 09:01

pratyakshsharma requested changes Jun 5, 2025

View reviewed changes

infvg force-pushed the db-session-property branch 2 times, most recently from f174a51 to fd53f8b Compare June 18, 2025 09:05

infvg requested review from elharo and steveburnett as code owners June 18, 2025 09:05

steveburnett requested changes Jun 18, 2025

View reviewed changes

infvg force-pushed the db-session-property branch 3 times, most recently from 78ac74c to 30a553c Compare June 23, 2025 08:01

steveburnett requested changes Jun 23, 2025

View reviewed changes

Comment thread presto-docs/src/main/sphinx/admin/session-property-managers.rst Outdated

infvg force-pushed the db-session-property branch from 30a553c to 4c6495f Compare June 25, 2025 06:07

steveburnett previously approved these changes Jun 25, 2025

View reviewed changes

infvg requested a review from imjalpreet July 11, 2025 14:12

imjalpreet requested changes Jul 15, 2025

View reviewed changes

imjalpreet reviewed Jul 15, 2025

View reviewed changes

infvg dismissed steveburnett’s stale review via dff7591 July 17, 2025 15:19

infvg force-pushed the db-session-property branch 2 times, most recently from 27e542f to f1c7154 Compare August 25, 2025 19:00

aaneja self-requested a review August 26, 2025 07:52

aaneja previously approved these changes Aug 26, 2025

View reviewed changes

imjalpreet reviewed Aug 26, 2025

View reviewed changes

infvg dismissed aaneja’s stale review via cddc79f August 26, 2025 10:04

infvg force-pushed the db-session-property branch 2 times, most recently from cddc79f to 2de1b16 Compare August 28, 2025 08:16

infvg requested a review from a team as a code owner August 28, 2025 08:16

infvg force-pushed the db-session-property branch 2 times, most recently from 7bbcbe2 to 9b01175 Compare August 28, 2025 08:35

imjalpreet reviewed Aug 28, 2025

View reviewed changes

infvg force-pushed the db-session-property branch 2 times, most recently from c2ab511 to a0f22ac Compare August 31, 2025 05:36

imjalpreet reviewed Sep 1, 2025

View reviewed changes

infvg and others added 2 commits September 1, 2025 13:05

Add a DB based session property manager

85c2664

Co-authored-by: Ashish Tadose <ashishtadose@gmail.com> Co-authored-by: Ariel Weisberg <aweisberg@fb.com> Co-authored-by: Jalpreet Singh Nanda (:imjalpreet) <jalpreetnanda@gmail.com>

imjalpreet approved these changes Sep 1, 2025

View reviewed changes

tdcmeehan approved these changes Sep 2, 2025

View reviewed changes

unidevel mentioned this pull request Sep 11, 2025

Add release notes for 0.295 unix280/presto#42

Closed

13 tasks

This was referenced Sep 24, 2025

Add release notes for 0.295 unix280/presto#43

Closed

Add release notes for 0.295 unix280/presto#45

Merged

unidevel mentioned this pull request Oct 1, 2025

Add release notes for 0.295 unix280/presto#46

Closed

17 tasks

prestodb-ci mentioned this pull request Oct 1, 2025

docs: Add release notes for 0.295 #26208

Merged

17 tasks


		.. code-block:: text

		presto:tpch> DESCRIBE session_client_tags;


		.. code-block:: text

		presto:tpch> DESCRIBE session_property_values;

		<artifactId>jakarta.validation-api</artifactId>
		</dependency>

Conversation

infvg commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

pratyakshsharma left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

infvg commented Apr 28, 2025 •

edited

Loading