You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vdiravka <gi...@git.apache.org> on 2018/03/24 00:22:52 UTC
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
GitHub user vdiravka opened a pull request:
https://github.com/apache/drill/pull/1189
DRILL-6282: Excluding io.dropwizard.metrics dependencies
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vdiravka/drill DRILL-6282
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1189.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1189
----
commit d0c6a5d6598ef384c1bca7f34db7923809f31980
Author: Vitalii Diravka <vi...@...>
Date: 2018-03-21T14:16:10Z
DRILL-6282: Excluding io.dropwizard.metrics dependencies
----
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1189
Please update JIRA, PR and commit titles and squash commits.
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1189
Why not make Drill to use `io.dropwizard.metrics` instead of `com.codahale.metrics`? As far as I can see it is the same library (this is why it causes the conflict).
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1189
@vdiravka right, check the first link, the new `groupId` for `com.codahale.metrics` is `io.dropwizard.metrics`, so all new versions will be deployed using the new `groupId`.
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r183870299
--- Diff: pom.xml ---
@@ -1333,6 +1353,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
--- End diff --
Where is the dependency used? Can it be replaced with the `io.dropwizard.metrics`?
---
[GitHub] drill issue #1189: DRILL-6282: Update Drill's Metrics dependencies
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1189
LGTM
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r183868650
--- Diff: pom.xml ---
@@ -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>
--- End diff --
define version as a property and avoid using `rc` version unless it provides a significant benefit.
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184144853
--- Diff: pom.xml ---
@@ -1333,6 +1353,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
--- End diff --
I am not sure why is it necessary to have `com.yammer.metrics` in dependencyManagement?
---
[GitHub] drill pull request #1189: DRILL-6282: Update Drill's Metrics dependencies
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1189
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184144021
--- Diff: logical/pom.xml ---
@@ -85,14 +85,12 @@
</dependency>
<dependency>
- <groupId>com.codahale.metrics</groupId>
+ <groupId>io.dropwizard.metrics</groupId>
--- End diff --
Is it used in logical?
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/1189
@vrozov I have replaced `com.codahale.metrics` with last `io.dropwizard.metrics`.
Please review.
---
[GitHub] drill pull request #1189: DRILL-6282: Update Drill's Metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184218887
--- Diff: pom.xml ---
@@ -1333,6 +1353,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
--- End diff --
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.
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184024460
--- Diff: pom.xml ---
@@ -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>
--- End diff --
ok, there is really no need to use `rc` version.
So the last stable [4.0.2](https://github.com/dropwizard/metrics/tree/4.1-development#version-4xx-javadoc). Let's choose this one.
---
[GitHub] drill pull request #1189: DRILL-6282: Excluding io.dropwizard.metrics depend...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184066086
--- Diff: pom.xml ---
@@ -1333,6 +1353,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
--- End diff --
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.
---
[GitHub] drill pull request #1189: DRILL-6282: Update Drill's Metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184223843
--- Diff: logical/pom.xml ---
@@ -85,14 +85,12 @@
</dependency>
<dependency>
- <groupId>com.codahale.metrics</groupId>
+ <groupId>io.dropwizard.metrics</groupId>
--- End diff --
No, it is. Thanks for catching this.
Also I have noticed that `java-exec` used `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).
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/1189
@vrozov Could you please review this, since you were the reviewer for DRILL-5978 (Hive upgrade)
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/1189
@vrozov Drill doesn't use `io.dropwizard.metrics` at all and it is in conflict with Drill's `com.codahale.metrics` (misspoke was in PR's description). Even Hive uses it only for own unit test, so there is no need to keep it in Drill for now.
---
[GitHub] drill pull request #1189: DRILL-6282: Update Drill's Metrics dependencies
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1189#discussion_r184243564
--- Diff: logical/pom.xml ---
@@ -85,14 +85,12 @@
</dependency>
<dependency>
- <groupId>com.codahale.metrics</groupId>
+ <groupId>io.dropwizard.metrics</groupId>
--- End diff --
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.
---
[GitHub] drill issue #1189: DRILL-6282: Excluding io.dropwizard.metrics dependencies
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/1189
@vrozov I understand what you mean: `com.codahale` isn't updated for a long time
https://mvnrepository.com/artifact/com.codahale.metrics/metrics-core,
but `io.dropwizard.metrics` is updated recently https://mvnrepository.com/artifact/io.dropwizard.metrics/metrics-core.
So we can switch into the last one. I will update my PR and let you know. Thank you.
---