Ensure module instances are not shared by multiple JDBC catalogs #24058
Conversation
| @@ -26,6 +26,7 @@ | |||
| import io.trino.spi.type.TypeManager; | |||
|
|
|||
| import java.util.Map; | |||
There was a problem hiding this comment.
If catalogs are loaded concurrently, this can lead to situations where a connector accesses the configuration of another connector.
Can you add a test to this PR showcasing this behavior?
There was a problem hiding this comment.
The problem is that this is a non-deterministic bug. A test I came up with takes around 20 seconds because it requires many iterations to hit this scenario. I'll try to come up with a better approach.
There was a problem hiding this comment.
I added a test. Without the changes from this PR, it consistently fails on my laptop when the number of iterations is set to 2000. However, this sometimes takes more than 20 seconds. I reduced the iterations to 100 to speed it up, so it may not always fail if the changes are not applied.
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java
Outdated
Show resolved
Hide resolved
|
Just couple of nitpicks |
When a JdbcPlugin is instantiated, creating an instance of a module at that time causes the instance to be shared by all connectors provided by the plugin. This is problematic when the module extends AbstractConfigurationAwareModule, as it holds a reference to the ConfigurationFactory, which is set dynamically during bootstrap. If catalogs are loaded concurrently, this can lead to situations where a connector accesses the configuration of another connector.
piotrrzysko
force-pushed
the
jdbc-module-leak
branch
from
33b6886
to
8ea280a
Compare
Description
When a
JdbcPluginis instantiated, creating an instance of a module at that time causes the instance to be shared by all connectors provided by the plugin. This is problematic when the module extendsAbstractConfigurationAwareModule, as it holds a reference to theConfigurationFactory, which is set dynamically during bootstrap. If catalogs are loaded concurrently, this can lead to situations where a connector accesses the configuration of another connector.Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: