-
Notifications
You must be signed in to change notification settings - Fork 330
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.
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
convert the build from ant/ivy to maven #70
Conversation
I'd love to get your feedback or review on this. Thanks! |
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<groupId>com.hadoop.gplcompression</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we change this to com.twitter if we plan to publish to maven central?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. It's more for the "community" to chime in. Should we change the group id? If so, to what?
Feedback on this? How about the group id? Shall we use a different group id than the current one? Changing the group id is fine, but the implication is we may have class collisions if an upstream project includes both this and the old hadoop-lzo (transitively). |
Ping? |
Sorry, can you rebase the changes so the pull request can be merged? If we don't use the 'com.twitter' groupId then we can't sync to central. I think it makes sense for us to use the com.twitter groupId because a) we kind of own this project b) we want to sync this to central |
Thanks Chris. The reason it cannot be merged automatically is not because it's not rebased properly but because a lot of files have been moved around (to conform to the standard maven directory layout). Once we resolve the group id discussion, I'd be happy to merge it myself. |
I am not really sure if it is acceptable to change the group id to com.twitter. How about we continue to publish to mvn.twitter.com for now as we have been doing as we look into it? |
|
||
<dependencies> | ||
<dependency> | ||
<groupId>org.apache.hadoop</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[from Raghu] for hadoop dependency we can use 'hadoop-client' rather than common & mapred-core etc.
in Elephant-bird: https://github.com/kevinweil/elephant-bird/blob/master/pom.xml#L246
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that. "Hadoop-client" is a superset of hadoop-common + hadoop-mapreduce-client-core. However, as good hygiene, I would prefer to describe the exact direct dependencies. That would tend to keep your transitive dependencies slim and prevent dependency creep. The actual implication of using hadoop-client over the current is to pull in some additional hadoop jars (hdfs, mapreduce-client-app, mapreduce-client-common, mapreduce-client-jobclient, mapreduce-client-shuffle, yarn-client, and yarn-server-common). Obviously the current hadoop-lzo code base does not use them...
+1. I would vote for merging it as is, and open a new issue for considering the group id change and continue the discussion. Thoughts? |
+1 for the new issue On Jun 20, 2013, at 5:58 PM, Sangjin Lee notifications@github.com wrote: +1. I would vote for merging it as is, and open a new issue for considering — |
I opened a new issue to discuss the group id issue. If you guys are OK with the changes, I'll merge this pull request. Let me know. |
+1 form me |
Cool, thanks. I'll go ahead and merge it today. |
This changes the build from ant/ivy to maven. I believe it replicates most (if not all) of the build functionalities in maven. It should not result in a materially different build result.