vladmihalcea / flexy-pool

FlexyPool adds metrics and failover strategies to a given Connection Pool, allowing it to resize on demand.
Apache License 2.0
1.09k stars 120 forks source link

Odd Dependency Structure within HikariCP JAR #45

Closed johnament closed 6 years ago

johnament commented 6 years ago

While starting to integrate flexy-pool with a Java SE app, I found the following CNFE:

Caused by: java.lang.NoClassDefFoundError: com/codahale/metrics/JmxReporter
    at com.vladmihalcea.flexypool.metric.codahale.JmxMetricReporter.init(JmxMetricReporter.java:28)
    at com.vladmihalcea.flexypool.metric.codahale.CodahaleMetrics.<init>(CodahaleMetrics.java:87)
    at com.vladmihalcea.flexypool.metric.codahale.CodahaleMetrics.<init>(CodahaleMetrics.java:102)
    at com.vladmihalcea.flexypool.metric.codahale.CodahaleMetrics$ReservoirMetricsFactory.newInstance(CodahaleMetrics.java:45)
    at com.vladmihalcea.flexypool.config.Configuration$Builder.build(Configuration.java:164)

This was confusing, since I didn't declare any metrics runtime (yet), and was planning to use DWM4.

The following is part of the dependency tree I see within my app:

[INFO] +- com.vladmihalcea.flexy-pool:flexy-hikaricp:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-pool-core:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-codahale-metrics:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-dropwizard-metrics:jar:1.3.0:compile
[INFO] |  \- io.dropwizard.metrics:metrics-core:jar:4.0.2:compile

I wouldn't expect a hard compile time dependency on these components, they don't seem like they're hard dependencies either within the code. So should they be removed? Or excluded?

vladmihalcea commented 6 years ago

You can have both flexy-pool:flexy-codahale-metrics and flexy-dropwizard-metrics jars in your project. Or you can exclude the flexy-codahale-metrics one.

The problem is that, on your classpath, you have the old Dorpwizard 3 library:

com/codahale/metrics/JmxReporter

And FlexyPool finds it at runtime and assumes that's what you want to use. Add a breakpoint in MetricsFactoryResolver class inside the resolve method and check what it finds.

Additionally, add a hard dependency on DW 4:

<dependency>
    <groupId>io.dropwizard.metrics</groupId>
    <artifactId>metrics-core</artifactId>
    <version>4.0.2</version>
</dependency>

The reason why there are two artifacts is that FlexyPool works with both DW3 and DW4. For more details, check out this article.

johnament commented 6 years ago

I do not have the Dropwizard 3 dependency on my classpath. Here's the full output of mvn dependency:tree

[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Building jpa-core 2.1-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- maven-dependency-plugin:2.8:tree (default-cli) @ jpa-core ---
[INFO] ws.ament.hammock:jpa-core:jar:2.1-SNAPSHOT
[INFO] +- org.eclipse.persistence:javax.persistence:jar:2.1.1:compile
[INFO] +- org.jboss.arquillian.junit:arquillian-junit-container:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.junit:arquillian-junit-core:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.test:arquillian-test-api:jar:1.2.0.Final:test
[INFO] |  |  \- org.jboss.arquillian.core:arquillian-core-api:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.test:arquillian-test-spi:jar:1.2.0.Final:test
[INFO] |  |  \- org.jboss.arquillian.core:arquillian-core-spi:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-test-api:jar:1.2.0.Final:test
[INFO] |  |  \- org.jboss.shrinkwrap:shrinkwrap-api:jar:1.2.6:test
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-test-spi:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.core:arquillian-core-impl-base:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.test:arquillian-test-impl-base:jar:1.2.0.Final:test
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-impl-base:jar:1.2.0.Final:test
[INFO] |  |  +- org.jboss.arquillian.config:arquillian-config-api:jar:1.2.0.Final:test
[INFO] |  |  +- org.jboss.arquillian.config:arquillian-config-impl-base:jar:1.2.0.Final:test
[INFO] |  |  \- org.jboss.shrinkwrap.descriptors:shrinkwrap-descriptors-spi:jar:2.0.0:test
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-test-impl-base:jar:1.2.0.Final:test
[INFO] |  \- org.jboss.shrinkwrap:shrinkwrap-impl-base:jar:1.2.6:test
[INFO] |     \- org.jboss.shrinkwrap:shrinkwrap-spi:jar:1.2.6:test
[INFO] +- junit:junit:jar:4.12:test
[INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] +- org.apache.geronimo.specs:geronimo-jta_1.2_spec:jar:1.0-alpha-1:test
[INFO] +- org.assertj:assertj-core:jar:3.8.0:test
[INFO] +- com.zaxxer:HikariCP:jar:2.7.4:compile
[INFO] |  \- org.slf4j:slf4j-api:jar:1.7.25:compile
[INFO] +- javax.annotation:javax.annotation-api:jar:1.3.1:compile
[INFO] +- com.h2database:h2:jar:1.4.196:test
[INFO] +- ws.ament.hammock:hammock-core:jar:2.1-SNAPSHOT:compile
[INFO] |  +- javax.inject:javax.inject:jar:1:compile
[INFO] |  +- org.jboss.logging:jboss-logging:jar:3.3.1.Final:compile
[INFO] |  +- org.apache.geronimo.config:geronimo-config-impl:jar:1.1:compile
[INFO] |  +- org.eclipse.microprofile.config:microprofile-config-api:jar:1.2:compile
[INFO] |  |  \- org.osgi:org.osgi.annotation.versioning:jar:1.0.0:compile
[INFO] |  +- org.apache.logging.log4j:log4j-slf4j-impl:jar:2.10.0:compile
[INFO] |  |  \- org.apache.logging.log4j:log4j-api:jar:2.10.0:compile
[INFO] |  +- org.apache.logging.log4j:log4j-core:jar:2.10.0:compile
[INFO] |  \- org.apache.logging.log4j:log4j-jul:jar:2.10.0:compile
[INFO] +- com.vladmihalcea.flexy-pool:flexy-hikaricp:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-pool-core:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-codahale-metrics:jar:1.3.0:compile
[INFO] |  +- com.vladmihalcea.flexy-pool:flexy-dropwizard-metrics:jar:1.3.0:compile
[INFO] |  \- io.dropwizard.metrics:metrics-core:jar:4.0.2:compile
[INFO] +- javax.enterprise:cdi-api:jar:2.0:compile
[INFO] |  +- javax.el:javax.el-api:jar:3.0.0:compile
[INFO] |  \- javax.interceptor:javax.interceptor-api:jar:1.2:compile
[INFO] +- org.jboss.arquillian.container:arquillian-weld-embedded:jar:2.0.0.Final:test
[INFO] |  +- org.jboss.arquillian.container:arquillian-container-spi:jar:1.2.0.Final:test
[INFO] |  |  \- org.jboss.shrinkwrap.descriptors:shrinkwrap-descriptors-api-base:jar:2.0.0:test
[INFO] |  \- org.jboss.arquillian.testenricher:arquillian-testenricher-cdi:jar:1.2.0.Final:test
[INFO] \- ws.ament.hammock:bootstrap-weld3:jar:2.1-SNAPSHOT:test
[INFO]    +- javax.servlet:javax.servlet-api:jar:3.1.0:test
[INFO]    +- org.jboss.weld.se:weld-se-core:jar:3.0.1.Final:test
[INFO]    |  +- org.jboss.weld.environment:weld-environment-common:jar:3.0.1.Final:test
[INFO]    |  |  \- org.jboss.weld:weld-core-impl:jar:3.0.1.Final:test
[INFO]    |  |     \- org.jboss.spec.javax.interceptor:jboss-interceptors-api_1.2_spec:jar:1.0.0.Final:test
[INFO]    |  +- org.jboss.weld.probe:weld-probe-core:jar:3.0.1.Final:test
[INFO]    |  \- org.jboss.classfilewriter:jboss-classfilewriter:jar:1.2.1.Final:test
[INFO]    \- org.jboss.weld.servlet:weld-servlet-core:jar:3.0.1.Final:test
[INFO]       +- org.jboss.weld:weld-spi:jar:3.0.SP1:test
[INFO]       |  \- org.jboss.weld:weld-api:jar:3.0.SP1:test
[INFO]       +- org.jboss.weld.module:weld-jsf:jar:3.0.1.Final:test
[INFO]       +- org.jboss.weld.module:weld-web:jar:3.0.1.Final:test
[INFO]       \- org.jboss.spec.javax.el:jboss-el-api_3.0_spec:jar:1.0.7.Final:test
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.191 s
[INFO] Finished at: 2018-01-21T10:22:01-05:00
[INFO] Final Memory: 17M/309M
[INFO] ------------------------------------------------------------------------

BTW, your prior answer was much more user friendly.

vladmihalcea commented 6 years ago

That's strange.

Add a breakpoint in MetricsFactoryResolver class inside the resolve method and try to find out why does the CodahaleMetricsFactoryService returns the CodahaleMetrics.FACTORY.

That should only happen if the com.codahale.metrics.Metric is located by the ClassLoader.

vladmihalcea commented 6 years ago

Ok, I investigated it and found the following:

  1. DP4 did not rename packages as they did in the 4.0.0-SNAPSHOP.
  2. They removed com.codahale.metrics.JmxReporter class.

For the moment, the only way to fix it is to use DW3 as DW4 is not supported.

johnament commented 6 years ago

Ok, either way I'm going with your original advice and just excluding the dependencies, and making sure users know to not include either of those dependencies.

It also makes me question if https://github.com/vladmihalcea/flexy-pool/commit/27d16d289f8c7cd0f353eef9f701c1317262015f is valid still?

vladmihalcea commented 6 years ago

So apparently this is what you need to do:

<dependency>
    <groupId>io.dropwizard.metrics</groupId>
    <artifactId>metrics-jmx</artifactId>
    <version>4.0.2</version>
</dependency>
vladmihalcea commented 6 years ago

It also makes me question if 27d16d2 is valid still?

Nope. It's not.

vladmihalcea commented 6 years ago

Closing this issue since I opened this one instead.

johnament commented 6 years ago

Either way, because of #46 I can't use your built in metrics support, since I want to use my CDI managed MetricRegistry instead of the one you're creating.

vladmihalcea commented 6 years ago

Did you check the comment related to customizing MetricFactory? Why wouldn't customize the MetricFactory work for you?

johnament commented 6 years ago

because the resolution of MetricsFactory happens in the constructor of Configuration.Builder.

java.lang.IllegalStateException: No MetricsFactory could be loaded!

    at com.vladmihalcea.flexypool.metric.MetricsFactoryResolver.resolve(MetricsFactoryResolver.java:33)
    at com.vladmihalcea.flexypool.config.Configuration$Builder.<init>(Configuration.java:37)
vladmihalcea commented 6 years ago

Good point. I'll move it into the build method and initialize it only if it's still a null reference.

vladmihalcea commented 6 years ago

I changed that as well. Check out the 2.0.0 version. Thanks for the indications you have given me. The dependencies are fixed as well. Only the one for Dropwizard 4 will be added now.

Check out this article.