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

Fix stats collection for JDBC based connector #3479

Merged
merged 1 commit into from Apr 28, 2020
Merged

Fix stats collection for JDBC based connector #3479

merged 1 commit into from Apr 28, 2020

Conversation

Praveen2112
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label Apr 19, 2020
kokosing
kokosing reviewed Apr 19, 2020
View reviewed changes
presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcDiagnosticModule.java
@@ -58,21 +58,19 @@ public void configure(Binder binder)
@StatsCollecting
public JdbcClient createJdbcClientWithStats(@ForBaseJdbc JdbcClient client)
{
StatisticsAwareJdbcClient statisticsAwareJdbcClient = new StatisticsAwareJdbcClient(client);
Copy link
Member

Choose a reason for hiding this comment

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

What was wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like StatisticsAwareJdbcClient#getStats is not exposed in JMX so the actual stats are not exposed

Before

   Column    |  Type   | Extra | Comment
-------------+---------+-------+---------
 node        | varchar |       |
 object_name | varchar |       |

After

                         Column                          |  Type   | Extra | Comment
---------------------------------------------------------+---------+-------+---------
 abortreadconnection.failures.fifteenminute.count        | double  |       |
 abortreadconnection.failures.fifteenminute.rate         | double  |       |
 abortreadconnection.failures.fiveminute.count           | double  |       |

Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test for that?

Or even better it would be to add a trap for things like that. Do you exact reasaon why stats were not exposed? Like not allowing to use newExporter when bean does not expose method with @Managed?

Copy link
Member

Choose a reason for hiding this comment

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

It didn't work because the StatisticsAwareJdbcClient instance was not a bean (never exposed at guice DI level) and so it could not get exported.

Copy link
Member

Choose a reason for hiding this comment

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

Stats do not get exposed automatically, we expose them using newExporter(..).... This code was called but StatisticsAwareJdbcClient was wrapped so annotated methods with @Managed were not accessible. It would be nice to have something to prevent such bugs.

@findepi
Copy link
Member

findepi commented Apr 19, 2020

please

  • change createJdbcClientWithStats return type to StatisticsAwareJdbcClient
  • change commit message to something like Expose JdbcClient invocation statistics in JMX

as this remains fragile still, can we have a test for this?

@Praveen2112 Praveen2112 force-pushed the jdbc_stat_changes branch 2 times, most recently from 00437cb to f7f9097 Compare April 19, 2020 16:44
@Praveen2112
Copy link
Member Author

@findepi Thanks for your feedback and for the commit message.

change createJdbcClientWithStats return type to StatisticsAwareJdbcClient

This might break DI there by JdbcClient might not be injected in CachingJdbcClient

@findepi
Copy link
Member

findepi commented Apr 19, 2020

This might break DI

you're right

kokosing
kokosing reviewed Apr 20, 2020
View reviewed changes
presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/TestingH2JdbcModule.java Outdated
@@ -26,7 +26,7 @@

import static java.lang.String.format;

class TestingH2JdbcModule
public class TestingH2JdbcModule
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously it was package protected so we cant access in our JmxTest or we have to move JmxTest to a different package

Copy link
Member

Choose a reason for hiding this comment

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

Please move JmxTest to different package. I don't think we need separate package for single test, do we? Otherwise please squash commits.

presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/jmx/TestJmxStats.java Outdated
{
@Test
public void testJmxStatsExposure()
throws MalformedObjectNameException, IntrospectionException, InstanceNotFoundException, ReflectionException
Copy link
Member

Choose a reason for hiding this comment

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

just throws Exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified

presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/jmx/TestJmxStats.java Outdated
ConnectorFactory factory = getOnlyElement(plugin.getConnectorFactories());
factory.create("test", ImmutableMap.of("connection-url", "jdbc"), new TestingConnectorContext());
MBeanServer mbeanServer = getPlatformMBeanServer();
for (String domainName : mbeanServer.getDomains()) {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this for

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it

presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/jmx/TestJmxStats.java Outdated
}
for (ObjectName objectName : mbeanServer.queryNames(new ObjectName("io.prestosql.plugin.jdbc:*"), null)) {
MBeanInfo mbeanInfo = mbeanServer.getMBeanInfo(objectName);
assertFalse(Arrays.isNullOrEmpty(mbeanInfo.getAttributes()), format("Object %s doesn't expose JMX stats", objectName.getCanonicalName()));
Copy link
Member

Choose a reason for hiding this comment

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

static import

presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/jmx/TestJmxStats.java Outdated Show resolved Hide resolved
@Praveen2112
Copy link
Member Author

@kokosing Thanks for your feedback. Have applied your comments

kokosing
kokosing reviewed Apr 20, 2020
View reviewed changes
presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/TestingH2JdbcModule.java Outdated
@@ -26,7 +26,7 @@

import static java.lang.String.format;

class TestingH2JdbcModule
public class TestingH2JdbcModule
Copy link
Member

Choose a reason for hiding this comment

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

Please move JmxTest to different package. I don't think we need separate package for single test, do we? Otherwise please squash commits.

presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/jmx/TestJmxStats.java Outdated
ConnectorFactory factory = getOnlyElement(plugin.getConnectorFactories());
factory.create("test", ImmutableMap.of("connection-url", "jdbc"), new TestingConnectorContext());
MBeanServer mbeanServer = getPlatformMBeanServer();
for (ObjectName objectName : mbeanServer.queryNames(new ObjectName("io.prestosql.plugin.jdbc:*"), null)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure that mbeanServer.queryNames(new ObjectName("io.prestosql.plugin.jdbc:*") does not return empty list. Also please verify that few that we care of are there (jdbc client and coonnection factory)

@Praveen2112 Praveen2112 force-pushed the jdbc_stat_changes branch 7 times, most recently from 0042c39 to 29ad571 Compare April 27, 2020 08:47
@Praveen2112 Praveen2112 merged commit 7d211ef into trinodb:master Apr 28, 2020
@Praveen2112 Praveen2112 deleted the jdbc_stat_changes branch April 28, 2020 10:41
@Praveen2112 Praveen2112 mentioned this pull request May 2, 2020
Closed
8 tasks
@electrum electrum added this to the 333 milestone May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi left review comments

@kokosing kokosing kokosing approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
333
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Praveen2112 @findepi @kokosing @electrum