-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add a DB based session property manager #24995
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.
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
base: master
Are you sure you want to change the base?
Conversation
b9191b4
to
a405418
Compare
064f35e
to
9f7283f
Compare
c335922
to
b29057b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f174a51
to
fd53f8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the documentation! A few suggestions.
presto-docs/src/main/sphinx/admin/session-property-managers.rst
Outdated
Show resolved
Hide resolved
presto-docs/src/main/sphinx/admin/session-property-managers.rst
Outdated
Show resolved
Hide resolved
presto-docs/src/main/sphinx/admin/session-property-managers.rst
Outdated
Show resolved
Hide resolved
presto-docs/src/main/sphinx/admin/session-property-managers.rst
Outdated
Show resolved
Hide resolved
78ac74c
to
30a553c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor phrasing suggestion.
presto-docs/src/main/sphinx/admin/session-property-managers.rst
Outdated
Show resolved
Hide resolved
30a553c
to
4c6495f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
Pull updated branch, new local doc build, looks good. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infvg, thank you, I finished my first pass, please have a look.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infvg I think we should also add a brief description of every new property introduced. @steveburnett, do you think that would be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imjalpreet yes, I agree. All properties should be documented. See Designing Your Code section iii in CONTRIBUTING.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
.. code-block:: text | ||
|
||
presto:tpch> DESCRIBE session_specs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table is present in the MySQL DB. The CLI prompt presto:tpch>
is a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should show output from MySQL CLI rather than Presto CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
|
||
.. code-block:: text | ||
|
||
presto:tpch> DESCRIBE session_client_tags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table is present in the MySQL DB. The CLI prompt presto:tpch>
is a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
|
||
.. code-block:: text | ||
|
||
presto:tpch> DESCRIBE session_property_values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table is present in the MySQL DB. The CLI prompt presto:tpch>
is a little confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note
import static com.facebook.presto.session.file.FileSessionPropertyManager.CODEC; | ||
import static org.testng.Assert.assertEquals; | ||
|
||
public class TestFileSessionPropertyManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a similar test for the DB-based session property manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests
@@ -659,6 +666,11 @@ public Optional<InternalResourceGroupManager<?>> getResourceGroupManager() | |||
return resourceGroupManager; | |||
} | |||
|
|||
public SessionPropertyDefaults getSessionPropertyDefaults() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the changes in this file? I don't see any usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any tests added yet that are using this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to similar mysql dependencies that are used in the new tests.
@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(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't override these. Based on what I understand, we can just override the abstract method: getSessionMatchSpecs
in all implementations of AbstractSessionPropertyManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infvg, please rebase on the latest master as well to trigger all tests.
6fb43f1
to
323cf08
Compare
86eb966
to
3a398c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infvg I did another pass, mostly good, a few suggestions.
Looks like there are some test failures. Please look into them. Thanks.
``session-property-manager.db.refresh-period`` should be set to how often presto refreshes to | ||
match the database. | ||
|
||
This database consists of three tables: session specs, client tags and properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's mention the actual table names like session_specs
, session_client_tags
, and session_property_values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
public static final String DEFAULT_PROPERTIES = "defaultProperties"; | ||
public static final String OVERRIDE_PROPERTIES = "overrideProperties"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can be private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
public SessionPropertyConfigurationManager create(Map<String, String> config, SessionPropertyConfigurationManagerContext context) | ||
{ | ||
try { | ||
Bootstrap app = new Bootstrap(new JsonModule(), new JsonModule(), new DbSessionPropertyManagerModule()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: repetitive JsonModule() argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
binder.bind(DbSessionPropertyManager.class).in(Scopes.SINGLETON); | ||
binder.bind(SessionPropertiesDao.class).toProvider(SessionPropertiesDaoProvider.class).in(Scopes.SINGLETON); | ||
binder.bind(DbSpecsProvider.class).to(RefreshingDbSpecsProvider.class).in(Scopes.SINGLETON); | ||
newExporter(binder).export(DbSpecsProvider.class).withGeneratedName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to expose any JMX metrics? Currently, I don't see anything exposed by RefreshingDbSpecsProvider
/DbSpecsProvider
. If not, we don't need this exporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
public TestDbSessionPropertyManager() | ||
throws Exception | ||
{ | ||
this.mysqlServer = new TestingMySqlServer("testuser", "testpass", ImmutableList.of(), MY_SQL_OPTIONS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's add an AfterClass tearDown method to close/stop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
884d625
to
a122789
Compare
Co-authored-by: Ashish Tadose <[email protected]> Co-authored-by: Ariel Weisberg <[email protected]> Co-authored-by: Jalpreet Singh Nanda (:imjalpreet) <[email protected]>
a122789
to
77c666d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, one last nit.
.. code-block:: none | ||
|
||
session-property-config.configuration-manager=db | ||
session-property-manager.db.url=url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: instead of url let's mention jdbc:mysql://localhost:3306/session_properties
to give an example of what is expected in this config
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
Release Notes