DRILL-6282: Update Drill's Metrics dependencies #1189
Conversation
|
@vrozov Could you please review this, since you were the reviewer for DRILL-5978 (Hive upgrade) |
|
@vrozov Drill doesn't use |
|
Why not make Drill to use |
|
@vrozov I understand what you mean: So we can switch into the last one. I will update my PR and let you know. Thank you. |
|
@vdiravka right, check the first link, the new |
|
@vrozov I have replaced |
pom.xml
Outdated
| @@ -1164,7 +1164,27 @@ | |||
| <dependency> | |||
| <groupId>io.dropwizard.metrics</groupId> | |||
| <artifactId>metrics-core</artifactId> | |||
| <version>4.0.2</version> | |||
| <version>4.1.0-rc0</version> | |||
There was a problem hiding this comment.
define version as a property and avoid using rc version unless it provides a significant benefit.
There was a problem hiding this comment.
ok, there is really no need to use rc version.
So the last stable 4.0.2. Let's choose this one.
pom.xml
Outdated
| @@ -1333,6 +1353,12 @@ | |||
| </exclusion> | |||
| </exclusions> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Where is the dependency used? Can it be replaced with the io.dropwizard.metrics?
There was a problem hiding this comment.
I thought that Drill uses it. But I have checked and found that there is no direct using of this kind of metrics. Therefore 4 dependencies blocks can be removed.
And now com.yammer.metrics is a transitive dependency for Drill and is a intra-project dependency for hbase-server, and kafka_2.11. Since the dependency doesn't conflict with io.dropwizard it would be good just to control the version of it in dependency-management block to avoid issues in future.
There was a problem hiding this comment.
I am not sure why is it necessary to have com.yammer.metrics in dependencyManagement?
There was a problem hiding this comment.
I meant that it can be useful if some new dependencies will have older transitive org.yummer.metrics in future.
But I do not mind, this isn't so important. Let's leave it without dependencyManagement control.
pom.xml
Outdated
| @@ -1164,7 +1164,27 @@ | |||
| <dependency> | |||
| <groupId>io.dropwizard.metrics</groupId> | |||
| <artifactId>metrics-core</artifactId> | |||
| <version>4.0.2</version> | |||
| <version>4.1.0-rc0</version> | |||
There was a problem hiding this comment.
ok, there is really no need to use rc version.
So the last stable 4.0.2. Let's choose this one.
pom.xml
Outdated
| @@ -1333,6 +1353,12 @@ | |||
| </exclusion> | |||
| </exclusions> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
I thought that Drill uses it. But I have checked and found that there is no direct using of this kind of metrics. Therefore 4 dependencies blocks can be removed.
And now com.yammer.metrics is a transitive dependency for Drill and is a intra-project dependency for hbase-server, and kafka_2.11. Since the dependency doesn't conflict with io.dropwizard it would be good just to control the version of it in dependency-management block to avoid issues in future.
logical/pom.xml
Outdated
| @@ -85,14 +85,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.codahale.metrics</groupId> | |||
| <groupId>io.dropwizard.metrics</groupId> | |||
There was a problem hiding this comment.
No, it is not. Thanks for catching this.
Also I have noticed that java-exec uses io.dropwizard.metrics as transitive from drill-common.
For consistency I have removed io.dropwizard.metrics from drill-memory-base (it can leverage metrics from drill-common too).
There was a problem hiding this comment.
It is not a best practice to rely on a transitive dependency for a compile scope dependency. I'd recommend specifying the dependency on io.dropwizard.metrics explicitly in case java-exec or drill-memory-base uses it.
|
Please update JIRA, PR and commit titles and squash commits. |
vdiravka
force-pushed
the
DRILL-6282
branch
2 times, most recently
from
26e6a7f
to
1fddec7
Compare
- Replacing com.codahale.metrics with last io.dropwizard.metrics Metrics for Drill - com.yammer.metrics is removed, since isn't used directly by Drill
vdiravka
changed the title
pom.xml
Outdated
| @@ -1333,6 +1353,12 @@ | |||
| </exclusion> | |||
| </exclusions> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
I meant that it can be useful if some new dependencies will have older transitive org.yummer.metrics in future.
But I do not mind, this isn't so important. Let's leave it without dependencyManagement control.
logical/pom.xml
Outdated
| @@ -85,14 +85,12 @@ | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.codahale.metrics</groupId> | |||
| <groupId>io.dropwizard.metrics</groupId> | |||
There was a problem hiding this comment.
No, it is not. Thanks for catching this.
Also I have noticed that java-exec uses io.dropwizard.metrics as transitive from drill-common.
For consistency I have removed io.dropwizard.metrics from drill-memory-base (it can leverage metrics from drill-common too).
|
LGTM |
- Replacing com.codahale.metrics with last io.dropwizard.metrics Metrics for Drill - com.yammer.metrics is removed, since isn't used directly by Drill closes apache#1189
- Replacing com.codahale.metrics with last io.dropwizard.metrics Metrics for Drill - com.yammer.metrics is removed, since isn't used directly by Drill closes apache#1189
Hive upgrade brings unnecessary
io.dropwizard.metricsdependencies for Drill. They are used only for Hive test purposes. Moreover they are in conflict with Drill'scom.codahale.metricsdependencies.