Collect execution stats for all JdbcClients #906

kokosing · 2019-06-04T14:02:22Z

Collect execution stats for PostgresSqlJdbcClient

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClientModule.java

electrum

This looks good overall, but we shouldn't limit it to PostgreSQL. It should also be simpler to expose this for all connectors in the base library.

electrum · 2019-06-05T00:17:45Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClientModule.java

+    @Inject
+    @Provides
+    @Singleton
+    public PostgreSqlClient getOracleClient(


electrum · 2019-06-05T00:28:15Z

I think we can do this by adding a binding annotation and provider

@Provides
@Singleton
@ForMetadataFactory
public static JdbcClient createJdbcClientWithStats(JdbcClient client)
{
    return new StatisticsAwareJdbcClient(client);
}

Then do

@Inject
public JdbcMetadataFactory(@ForMetadataFactory JdbcClient jdbcClient, JdbcMetadataConfig config)
{
    ...
}

kokosing · 2019-06-05T08:39:09Z

@electrum Thanks to your comment, I enabled this for all connectors.

Here is an example output for postgresql connector (captured with presto-cli/target/presto-cli-314-SNAPSHOT-executable.jar --server http://127.0.0.1:52280 --execute 'select * from jmx.current."io.prestosql.plugin.jdbc:name=postgresql,type=jdbcclient";' --output-format JSON | jq . ): https://gist.github.com/kokosing/41a958909b20262cba1c667ac0840fb5

kokosing · 2019-06-05T08:42:14Z

btw, worth to notice:

  "getcolumns.time.alltime.avg": 10.671700698381878,
  "getcolumns.time.alltime.count": 1545,
  "getcolumns.time.alltime.max": 35.289265,

electrum · 2019-06-07T07:49:53Z

The "Do not call ..." commit message title is too long

electrum · 2019-06-07T08:24:31Z

Add ConnectorObjectNameGeneratorModule for this, so that the names are exported with a base name of presto.plugin.mysql (or similar) like we do for others.

I'm not sure why we have multiple copies of that. It can probably live in presto-plugin-toolkit, but we can clean that up later.

electrum

This is looking good. I like the cleanup and injection of ConnectionFactory. These stats will be very useful for tracking performance issues.

presto-base-jdbc/pom.xml

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcConnectorFactory.java

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/guice/InternalBaseJdbc.java

presto-mysql/src/main/java/io/prestosql/plugin/mysql/MySqlClientModule.java

...e-jdbc/src/main/java/io/prestosql/plugin/jdbc/caching/TransactionScopeCachingJdbcClient.java

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadataFactory.java

presto-phoenix/src/main/java/io/prestosql/plugin/phoenix/PhoenixClientModule.java

kokosing

@electrum Comments addressed

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcConnectorFactory.java

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/guice/InternalBaseJdbc.java

presto-mysql/src/main/java/io/prestosql/plugin/mysql/MySqlClient.java

kokosing · 2019-06-24T19:13:18Z

@electrum ping

electrum

Looks great! A few minor comments

electrum · 2019-06-27T17:50:32Z

presto-base-jdbc/pom.xml

+        </dependency>
+
+        <!-- used by tests but also needed transitively -->
+        <dependency>


I think this needs <scope>runtime</scope> otherwise the dependency checker will fail as it's not referenced

electrum · 2019-06-27T17:55:06Z

presto-base-jdbc/pom.xml

@@ -100,6 +105,12 @@
            <artifactId>jackson-databind</artifactId>
        </dependency>

+        <!-- used by tests but also needed transitively -->
+        <dependency>


Probably needs runtime scope

electrum · 2019-06-27T17:56:20Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcModule.java

@@ -37,5 +52,19 @@ public void configure(Binder binder)
        binder.bind(JdbcPageSinkProvider.class).in(Scopes.SINGLETON);
        binder.bind(JdbcConnector.class).in(Scopes.SINGLETON);
        configBinder(binder).bindConfig(JdbcMetadataConfig.class);
+
+        // JMX
+        install(new MBeanServerModule());


Move these to JdbcConnectorFactory. We normally install modules at the top level.

electrum · 2019-06-27T17:58:06Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/TransactionScopeCachingJdbcClient.java

+    @Override
+    public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHandle tableHandle)
+    {
+        return getColumnsCache.computeIfAbsent(tableHandle, jdbcTableHandle -> super.getColumns(session, jdbcTableHandle));


Maybe shorten lambda variable to handle. Or make it ignored and use tableHandle in the lambda.

electrum · 2019-06-27T18:00:33Z

...o-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/jmx/StatisticsAwareConnectionFactory.java

+public class StatisticsAwareConnectionFactory
+        implements ConnectionFactory
+{
+    private final ConnectionFactoryStats stats = new ConnectionFactoryStats();


We could simplify this by inlining the stats fields, since there are only two of them. Up to you.

electrum · 2019-06-27T18:07:21Z

presto-phoenix/src/main/java/io/prestosql/plugin/phoenix/PhoenixConfig.java

@@ -83,4 +89,27 @@ public PhoenixConfig setCaseInsensitiveNameMatchingCacheTtl(Duration caseInsensi
        this.caseInsensitiveNameMatchingCacheTtl = caseInsensitiveNameMatchingCacheTtl;
        return this;
    }
+
+    public Properties getConnectionProperties()


Make this a private method in PhoenixClientModule. Config classes should be simple beans.

It cannot be private as it is also used by PhoenixClient

- remove redundant parameter - remove redundant throws

cla-bot bot added the cla-signed label Jun 4, 2019

kokosing added the WIP label Jun 4, 2019

kokosing requested review from electrum and findepi June 4, 2019 14:02

kokosing commented Jun 4, 2019

View reviewed changes

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClientModule.java Outdated Show resolved Hide resolved

kokosing force-pushed the origin/master/118_jdbc_stats branch from 8680ab2 to a531654 Compare June 4, 2019 18:35

electrum reviewed Jun 5, 2019

View reviewed changes

kokosing force-pushed the origin/master/118_jdbc_stats branch from a531654 to 2336293 Compare June 5, 2019 08:37

kokosing removed the WIP label Jun 5, 2019

kokosing changed the title ~~Collect execution stats for PostgresSqlJdbcClient~~ Collect execution stats for all JdbcClient Jun 5, 2019

kokosing changed the title ~~Collect execution stats for all JdbcClient~~ Collect execution stats for all JdbcClients Jun 5, 2019

electrum reviewed Jun 7, 2019

View reviewed changes

electrum assigned kokosing Jun 12, 2019

kokosing force-pushed the origin/master/118_jdbc_stats branch from 9535bf7 to e191376 Compare June 17, 2019 19:44

kokosing commented Jun 17, 2019

View reviewed changes

kokosing assigned electrum and unassigned kokosing Jun 24, 2019

electrum approved these changes Jun 27, 2019

View reviewed changes

electrum removed their assignment Jun 27, 2019

kokosing force-pushed the origin/master/118_jdbc_stats branch from e191376 to 3007059 Compare June 28, 2019 13:24

kokosing added 4 commits July 3, 2019 14:52

Add jol-core SPI provided dependency to plugins

60f83bc

Mark SPI dependencies as provided for plugins

e3a7777

Cleanup code

a9bc767

- remove redundant parameter - remove redundant throws

Introduce ForwardingJdbcClient

bc98c6d

kokosing added 5 commits July 3, 2019 14:52

Introduce StatisticsAwareJdbcClient

567bf42

Collect execution stats for JdbcClient for all connectors

3c72dda

Inject ConnectionFactory in Jdbc based connectors

f21788b

Collect stats for ConnectionFactory

0ad4ca7

Avoid extra JdbcClient::getColumns calls

9b27daa

kokosing force-pushed the origin/master/118_jdbc_stats branch from 3007059 to 9b27daa Compare July 3, 2019 13:12

kokosing merged commit 625bdf6 into trinodb:master Jul 4, 2019

kokosing deleted the origin/master/118_jdbc_stats branch July 4, 2019 11:55

kokosing mentioned this pull request Jul 4, 2019

Release notes for 316 #1000

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect execution stats for all JdbcClients #906

Collect execution stats for all JdbcClients #906

kokosing commented Jun 4, 2019

electrum left a comment

electrum Jun 5, 2019

electrum commented Jun 5, 2019

kokosing commented Jun 5, 2019 •

edited

Loading

kokosing commented Jun 5, 2019

electrum commented Jun 7, 2019

electrum commented Jun 7, 2019

electrum left a comment

kokosing left a comment

kokosing commented Jun 24, 2019

electrum left a comment

electrum Jun 27, 2019

electrum Jun 27, 2019

electrum Jun 27, 2019

electrum Jun 27, 2019

electrum Jun 27, 2019

electrum Jun 27, 2019

kokosing Jun 28, 2019

Collect execution stats for all JdbcClients #906

Collect execution stats for all JdbcClients #906

Conversation

kokosing commented Jun 4, 2019

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Jun 5, 2019

kokosing commented Jun 5, 2019 • edited Loading

kokosing commented Jun 5, 2019

electrum commented Jun 7, 2019

electrum commented Jun 7, 2019

electrum left a comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

kokosing commented Jun 24, 2019

electrum left a 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

kokosing commented Jun 5, 2019 •

edited

Loading