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.

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

Collect execution stats for all JdbcClients #906

Merged
merged 9 commits into from Jul 4, 2019

Conversation

2 participants
@kokosing
Copy link
Member

commented Jun 4, 2019

Collect execution stats for PostgresSqlJdbcClient

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

@kokosing kokosing added the WIP label Jun 4, 2019

@kokosing kokosing requested review from electrum and findepi Jun 4, 2019

@kokosing kokosing force-pushed the kokosing:origin/master/118_jdbc_stats branch from 8680ab2 to a531654 Jun 4, 2019

@electrum
Copy link
Member

left a comment

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.

@Inject
@Provides
@Singleton
public PostgreSqlClient getOracleClient(

This comment has been minimized.

Copy link
@electrum

electrum Jun 5, 2019

Member

Misnamed

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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 kokosing force-pushed the kokosing:origin/master/118_jdbc_stats branch from a531654 to 2336293 Jun 5, 2019

@kokosing

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

btw, worth to notice:

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

@kokosing kokosing removed the WIP label Jun 5, 2019

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

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

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

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

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

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
Copy link
Member

left a comment

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

@kokosing kokosing force-pushed the kokosing:origin/master/118_jdbc_stats branch from 9535bf7 to e191376 Jun 17, 2019

@kokosing
Copy link
Member Author

left a comment

@electrum Comments addressed

@kokosing

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

@electrum ping

@kokosing kokosing assigned electrum and unassigned kokosing Jun 24, 2019

@electrum
Copy link
Member

left a comment

Looks great! A few minor comments

</dependency>

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

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

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

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

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

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

Probably needs runtime scope

@@ -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());

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

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

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

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

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

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

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

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

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

public Properties getConnectionProperties()

This comment has been minimized.

Copy link
@electrum

electrum Jun 27, 2019

Member

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

This comment has been minimized.

Copy link
@kokosing

kokosing Jun 28, 2019

Author Member

It cannot be private as it is also used by PhoenixClient

@electrum electrum removed their assignment Jun 27, 2019

@kokosing kokosing force-pushed the kokosing:origin/master/118_jdbc_stats branch from e191376 to 3007059 Jun 28, 2019

@kokosing kokosing force-pushed the kokosing:origin/master/118_jdbc_stats branch from 3007059 to 9b27daa Jul 3, 2019

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

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@kokosing kokosing deleted the kokosing:origin/master/118_jdbc_stats branch Jul 4, 2019

@kokosing kokosing referenced this pull request Jul 4, 2019

Closed

Release notes for 316 #1000

5 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.