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

Jump to bottom

Implement getCatalogSessionProperties #5330

Merged
merged 5 commits into from
Oct 12, 2020
Merged

Implement getCatalogSessionProperties #5330

merged 5 commits into from
Oct 12, 2020

Conversation

rclaude
Copy link
Contributor

@rclaude rclaude commented Sep 28, 2020

What: Add support for setting default catalog session properties in FileSessionPropertyManager & DbSessionPropertyManager
How: Use the convention that the first dot in a property name is a separator between a catalog name and a the catalog specific property name

This PR is a replacement for #5188

@cla-bot cla-bot bot added the cla-signed label Sep 28, 2020
@rclaude rclaude mentioned this pull request Sep 28, 2020
Closed
@findepi findepi requested review from phd3 and kokosing September 28, 2020 21:01
@rclaude rclaude force-pushed the noapichange branch 2 times, most recently from e7af8ae to b8fe917 Compare September 29, 2020 14:29
phd3
phd3 reviewed Sep 30, 2020
View reviewed changes
Copy link
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

Thanks @rclaude, this looks good. Just a few minor comments.

...erty-managers/src/main/java/io/prestosql/plugin/session/file/FileSessionPropertyManager.java Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/session-property-managers.rst Show resolved Hide resolved
...perty-managers/src/main/java/io/prestosql/plugin/session/AbstractSessionPropertyManager.java Outdated Show resolved Hide resolved
...erty-managers/src/test/java/io/prestosql/plugin/session/db/TestDbSessionPropertyManager.java Outdated Show resolved Hide resolved
@rclaude rclaude force-pushed the noapichange branch 4 times, most recently from c15237e to 89339eb Compare October 6, 2020 07:01
@rclaude
Copy link
Contributor Author

rclaude commented Oct 6, 2020 •
edited
Loading

I tried to rebase but still face ci errors that are unrelated with the PR, don't know how to go about that.
This time it's:

tests               | 2020-10-06 13:19:24 INFO: Testing with bucketingType=BUCKETED_V1, value='prestosql rocks', insertWithPresto=false, expectedFileNamePossibilites=[000001_0]
tests               | 2020-10-06 13:19:25 INFO: FAILURE     /    io.prestosql.tests.hive.TestHiveBucketedTables.testBucketingVersion (Groups: ) took 49.2 seconds
tests               | 2020-10-06 13:19:25 SEVERE: Failure cause:
tests               | io.prestosql.tempto.query.QueryExecutionException: java.sql.SQLException: Error while processing statement: FAILED: Execution Error, return code 1 from org.apache.hadoop.hive.ql.exec.mr.MapRedTask. Error caching map.xml: java.nio.channels.ClosedByInterruptException
tests               | 	at io.prestosql.tempto.query.JdbcQueryExecutor.execute(JdbcQueryExecutor.java:114)
tests               | 	at io.prestosql.tempto.query.JdbcQueryExecutor.executeQuery(JdbcQueryExecutor.java:82)
tests               | 	at io.prestosql.tests.hive.TestHiveBucketedTables.testBucketingVersion(TestHiveBucketedTables.java:304)
tests               | 	at io.prestosql.tests.hive.TestHiveBucketedTables.testBucketingVersion(TestHiveBucketedTables.java:280)
tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
tests               | 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
tests               | 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
tests               | 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
tests               | 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
tests               | 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:645)
tests               | 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851)
tests               | 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177)
tests               | 	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
tests               | 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
tests               | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
tests               | 	at java.base/java.lang.Thread.run(Thread.java:834)
tests               | Caused by: java.sql.SQLException: Error while processing statement: FAILED: Execution Error, return code 1 from org.apache.hadoop.hive.ql.exec.mr.MapRedTask. Error caching map.xml: java.nio.channels.ClosedByInterruptException
tests               | 	at org.apache.hive.jdbc.HiveStatement.execute(HiveStatement.java:275)
tests               | 	at io.prestosql.tempto.query.JdbcQueryExecutor.executeQueryNoParams(JdbcQueryExecutor.java:122)
tests               | 	at io.prestosql.tempto.query.JdbcQueryExecutor.execute(JdbcQueryExecutor.java:107)
tests               | 	... 16 more

@rclaude rclaude requested a review from kokosing October 6, 2020 15:33
@kokosing kokosing mentioned this pull request Oct 6, 2020
Closed
kokosing
kokosing approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Please rebase and apply last comments. It looks like you hit #5427, hopefully after the rebase the issue won't appear again.

...erty-managers/src/test/java/io/prestosql/plugin/session/db/TestDbSessionPropertyManager.java Outdated Show resolved Hide resolved
...erty-managers/src/test/java/io/prestosql/plugin/session/db/TestDbSessionPropertyManager.java Outdated Show resolved Hide resolved
rclaude added 5 commits October 8, 2020 15:07
@rclaude
Refactor DB & File SessionPropertyManagers
3b096cb 
Create AbstractSessionPropertyManager to share common code in FileSessionPropertyManager and DbSessionPropertyManager
@rclaude
Filter system session properties
f31af65 
Use the convention that the first dot in a property name is a separator between a catalog name and a the catalog specific property name.
Thus only return session properties with name containing no dot.
@rclaude
Implement getCatalogSessionProperties
0730d07 
What: Add support for setting default catalog session properties in FileSessionPropertyManager and DbSessionPropertyManager
How: Use the convention that the first dot in a property name is a separator between a catalog name and a the catalog specific property name
@rclaude
Update session property managers documentation
21ad036 
* Add default catalog property configuration example
* Remove limitation section
@rclaude
Add AbstractTestSessionPropertyManager tests
2b6af84 
Test empty catalog and proprety names: handled transparently and not filtered out
Split SessionPropertyManagers testMultipleMatch
* Keep testing assembling of session properties from different matcher in testMultipleMatch
* Test "latter matcher override earlier ones" behavior for system and catalog properties in dedicated tests.
@rclaude
Copy link
Contributor Author

rclaude commented Oct 12, 2020

Please rebase and apply last comments. It looks like you hit #5427, hopefully after the rebase the issue won't appear again.

Got a green CI !

@kokosing kokosing merged commit 973cbd7 into trinodb:master Oct 12, 2020
@kokosing
Copy link
Member

Merged, thanks!

@kokosing kokosing mentioned this pull request Oct 12, 2020
Closed
7 tasks
@martint martint added this to the 345 milestone Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phd3 phd3 phd3 left review comments

@kokosing kokosing kokosing approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
345
Development

Successfully merging this pull request may close these issues.
4 participants
@rclaude @kokosing @phd3 @martint