You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/05/31 20:40:12 UTC
[GitHub] [helix] pkuwm opened a new pull request #1045: Update ivy dependencies to be same as pom dependencies
pkuwm opened a new pull request #1045:
URL: https://github.com/apache/helix/pull/1045
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1044
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Helix jars built based on ivy dependencies don't have the same dependencies as open source helix jars built with pom files. Eg., open source helix-core-1.0.0.jar depends on helix-common-1.0.0.jar, but the helix-core jar built with ivy files does't have such dependency.
This issue could cause inconsistent behaviors between internal helix lib release and open source lib release. Eg., Duplicate and Conflicting Class MonitorDomainNames happened in open source but not in internal release built with ivy dependencies.
We would like to make these dependencies consistent so issues like the example could be caught in release certification.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
Running...
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433000287
##########
File path: helix-core/pom.xml
##########
@@ -128,30 +128,10 @@ under the License.
<artifactId>commons-math3</artifactId>
<version>3.6.1</version>
</dependency>
- <dependency>
- <groupId>commons-codec</groupId>
- <artifactId>commons-codec</artifactId>
- <version>1.6</version>
- </dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <version>15.0</version>
- </dependency>
<dependency>
<groupId>org.yaml</groupId>
<artifactId>snakeyaml</artifactId>
- <version>1.12</version>
- </dependency>
- <dependency>
- <groupId>io.dropwizard.metrics</groupId>
- <artifactId>metrics-core</artifactId>
- <version>3.2.3</version>
- </dependency>
- <dependency>
- <groupId>org.apache.httpcomponents</groupId>
- <artifactId>httpclient</artifactId>
- <version>4.5.8</version>
+ <version>1.17</version>
Review comment:
Same comment on yaml. Although with YAML, I think it should be fine to upgrade. It's not as crucial to Helix pipeline as Jackson.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r438482182
##########
File path: metrics-common/metrics-common-0.9.2-SNAPSHOT.ivy
##########
@@ -43,5 +43,6 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
+ <dependency org="io.dropwizard.metrics" name="metrics-core" rev="3.2.3" conf="compile->compile(*),master(*);runtime->runtime(*)/>
Review comment:
It is to make sure jars built with ivy have the same dependencies with jars built with maven pom.
##########
File path: helix-core/pom.xml
##########
@@ -128,30 +128,10 @@ under the License.
<artifactId>commons-math3</artifactId>
<version>3.6.1</version>
</dependency>
- <dependency>
- <groupId>commons-codec</groupId>
- <artifactId>commons-codec</artifactId>
- <version>1.6</version>
- </dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <version>15.0</version>
- </dependency>
<dependency>
<groupId>org.yaml</groupId>
<artifactId>snakeyaml</artifactId>
- <version>1.12</version>
- </dependency>
- <dependency>
- <groupId>io.dropwizard.metrics</groupId>
- <artifactId>metrics-core</artifactId>
- <version>3.2.3</version>
- </dependency>
- <dependency>
- <groupId>org.apache.httpcomponents</groupId>
- <artifactId>httpclient</artifactId>
- <version>4.5.8</version>
+ <version>1.17</version>
Review comment:
Same. It is because "open source libraries aren't always backward compatible and could cause unwarranted code behavior change.". We have different dependency versions for Jackson-core-asl. In run time, this version difference may cause conflict and exceptions like NoSuchClassException. I actually would like to upgrade it to make sure only one version is used in our project to avoid conflicts.
##########
File path: helix-admin-webapp/pom.xml
##########
@@ -79,12 +79,12 @@ under the License.
<dependency>
<groupId>org.codehaus.jackson</groupId>
<artifactId>jackson-core-asl</artifactId>
- <version>1.8.5</version>
Review comment:
It is because "open source libraries aren't always backward compatible and could cause unwarranted code behavior change.". We have different dependency versions for Jackson-core-asl. In run time, this version difference may cause conflict and exceptions like `NoSuchClassException`. I actually would like to upgrade it to make sure only one version is used in our project to avoid conflicts.
##########
File path: zookeeper-api/zookeeper-api-1.0.1-SNAPSHOT.ivy
##########
@@ -43,9 +43,10 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
- <dependency org="org.codehaus.jackson" name="jackson-core-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="commons-cli" name="commons-cli" rev="1.2" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.apache.httpcomponents" name="httpclient" rev="4.5.8" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metrics-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metadata-store-directory-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.zookeeper" name="zookeeper" rev="3.4.13" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.9.13" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="commons-codec" name="commons-codec" rev="1.14" conf="compile->compile(default);runtime->runtime(default);default->default"/>
Review comment:
It is to make sure jars built with ivy have the same dependencies with jars built with maven pom.
##########
File path: helix-common/helix-common-1.0.1-SNAPSHOT.ivy
##########
@@ -43,9 +43,7 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
- <dependency org="org.yaml" name="snakeyaml" rev="1.12" conf="compile->compile(default);runtime->runtime(default);default->default"/>
- <dependency org="org.codehaus.jackson" name="jackson-core-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="commons-cli" name="commons-cli" rev="1.2" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metrics-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="zookeeper-api" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
Review comment:
It is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1045:
URL: https://github.com/apache/helix/pull/1045
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r432999924
##########
File path: helix-admin-webapp/pom.xml
##########
@@ -79,12 +79,12 @@ under the License.
<dependency>
<groupId>org.codehaus.jackson</groupId>
<artifactId>jackson-core-asl</artifactId>
- <version>1.8.5</version>
Review comment:
Is this version bump up necessary?
Sometimes open source libraries aren't always backward compatible and could cause unwarranted code behavior change. So if there's no clear reason, we'd prefer to keep the version we know to be working.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1045:
URL: https://github.com/apache/helix/pull/1045#issuecomment-662847620
Verified. Result is in the description pic.
This PR is ready to be merged, approved by @jiajunwang
Helix jars built based on ivy dependencies don't have the same dependencies as open source helix jars built with pom files.
This issue could cause inconsistent behaviors between internal helix lib release and open source lib release. So release certification doesn't catch the dependency issue. This commit updates ivy files to make dependencies same as pom.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433000497
##########
File path: metrics-common/metrics-common-0.9.2-SNAPSHOT.ivy
##########
@@ -43,5 +43,6 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
+ <dependency org="io.dropwizard.metrics" name="metrics-core" rev="3.2.3" conf="compile->compile(*),master(*);runtime->runtime(*)/>
Review comment:
I am not sure if this library has been ELR-ed. We may not need to include this because we don't need this available other than build time to create a JAR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433000087
##########
File path: helix-common/helix-common-1.0.1-SNAPSHOT.ivy
##########
@@ -43,9 +43,7 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
- <dependency org="org.yaml" name="snakeyaml" rev="1.12" conf="compile->compile(default);runtime->runtime(default);default->default"/>
- <dependency org="org.codehaus.jackson" name="jackson-core-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="commons-cli" name="commons-cli" rev="1.2" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metrics-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="zookeeper-api" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
Review comment:
I'm not sure if this is the correct version we should use in .ivy file. Could you double check that our version bump up script replaces these versions to the correct one at build time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433000497
##########
File path: metrics-common/metrics-common-0.9.2-SNAPSHOT.ivy
##########
@@ -43,5 +43,6 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
+ <dependency org="io.dropwizard.metrics" name="metrics-core" rev="3.2.3" conf="compile->compile(*),master(*);runtime->runtime(*)/>
Review comment:
I am not sure if this library would be needed in the ivy file. We may not need to include this because we don't need this available other than build time to create a JAR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433000287
##########
File path: helix-core/pom.xml
##########
@@ -128,30 +128,10 @@ under the License.
<artifactId>commons-math3</artifactId>
<version>3.6.1</version>
</dependency>
- <dependency>
- <groupId>commons-codec</groupId>
- <artifactId>commons-codec</artifactId>
- <version>1.6</version>
- </dependency>
- <dependency>
- <groupId>com.google.guava</groupId>
- <artifactId>guava</artifactId>
- <version>15.0</version>
- </dependency>
<dependency>
<groupId>org.yaml</groupId>
<artifactId>snakeyaml</artifactId>
- <version>1.12</version>
- </dependency>
- <dependency>
- <groupId>io.dropwizard.metrics</groupId>
- <artifactId>metrics-core</artifactId>
- <version>3.2.3</version>
- </dependency>
- <dependency>
- <groupId>org.apache.httpcomponents</groupId>
- <artifactId>httpclient</artifactId>
- <version>4.5.8</version>
+ <version>1.17</version>
Review comment:
Same comment on yaml. Although with YAML, I think it should be fine to upgrade. It's not as crucial to Helix pipeline as Jackson. "If it ain't broke, don't fix it" works with open source libraries.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1045: Update ivy dependencies to be same as pom dependencies
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1045:
URL: https://github.com/apache/helix/pull/1045#discussion_r433001475
##########
File path: zookeeper-api/zookeeper-api-1.0.1-SNAPSHOT.ivy
##########
@@ -43,9 +43,10 @@ under the License.
<dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.14" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)">
<artifact name="slf4j-log4j12" ext="jar"/>
</dependency>
- <dependency org="org.codehaus.jackson" name="jackson-core-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.8.5" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="commons-cli" name="commons-cli" rev="1.2" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
- <dependency org="org.apache.httpcomponents" name="httpclient" rev="4.5.8" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metrics-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.helix" name="metadata-store-directory-common" rev="1.0.1-SNAPSHOT" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.apache.zookeeper" name="zookeeper" rev="3.4.13" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="org.codehaus.jackson" name="jackson-mapper-asl" rev="1.9.13" force="true" conf="compile->compile(*),master(*);runtime->runtime(*)"/>
+ <dependency org="commons-codec" name="commons-codec" rev="1.14" conf="compile->compile(default);runtime->runtime(default);default->default"/>
Review comment:
Probably not necessary for the same reason for metrics-core above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org