-
Notifications
You must be signed in to change notification settings - Fork 78
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
fixes #980 #995
fixes #980 #995
Conversation
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.
I double checked that there shouldn't be any need to update the LICENSE and/or NOTICE file with this change... as long as upstream Thrift has kept their LICENSE and NOTICE files accurate for the libthrift jar.
But, we still need to do relocations of the libthrift classes, and fix up the any class path issues with the naming of the final jar with the shaded content.
It couldn't hurt to generate a diff of the resulting jar file listings before and after this change. I'm mentioning that here so I don't forget to do it in a subsequent review.
modules/core/pom.xml
Outdated
<minimizeJar>true</minimizeJar> | ||
<artifactSet> | ||
<includes> | ||
<include>org.apache.thrift:libthrift</include> |
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.
Perhaps the original issue should have been more specific, but the intent was to relocate the libthrift classes. If we don't do the relocation, then the original conflict with Accumulo's libthrift dependency will not be resolved.
modules/core/pom.xml
Outdated
<shadedClassifierName>shaded</shadedClassifierName> | ||
<filters> | ||
<filter> | ||
<artifact>org.apache.thrift:libthrift</artifact> |
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.
I don't think this filter is doing anything, since it's not actually inluding/excluding any resource patterns.
modules/core/pom.xml
Outdated
</includes> | ||
</artifactSet> | ||
<shadedArtifactAttached>true</shadedArtifactAttached> | ||
<shadedClassifierName>shaded</shadedClassifierName> |
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.
Do we want to modify our original jar, or have the shaded one separate? I'm thinking we probably want the shaded one to be the default artifact, for this module, rather than a separate artifact with a classifier.
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.
I also think it would be best to modify the original jar instead of create a separate shaded jar.
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.
@cjmctague are you still working on this? I was asking because I was thinking it may be nice to get this in 1.2.0.
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.
Also, feel free to ask any quesitons I will help the best I can. I have used the shade plugin a few times, but not recently and not for this specific case.
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.
@keith-turner Yes I was still working on this. Currently in Hawaii kinda off the grid for most of the day for class. Survived the "Incoming ballistic missile" false alarm though.
I have it shaded and relocated but it is creating an uber-jar in fluo-core that has all the dependancies from the entire project and not just just that module so I want to fix that at some point.
I could probably vpn into my workstation later and push what I have changed.
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.
@mikewalch nice eye catching the lysdexic mistakes lately, glad I'm not the
only one now!
…On Jan 2, 2018 9:10 AM, "Keith Turner" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In modules/core/pom.xml
<#995 (comment)>:
> + <artifactId>maven-shade-plugin</artifactId>
+ <executions>
+ <execution>
+ <goals>
+ <goal>shade</goal>
+ </goals>
+ <phase>package</phase>
+ <configuration>
+ <minimizeJar>true</minimizeJar>
+ <artifactSet>
+ <includes>
+ <include>org.apache.thrift:libthrift</include>
+ </includes>
+ </artifactSet>
+ <shadedArtifactAttached>true</shadedArtifactAttached>
+ <shadedClassifierName>shaded</shadedClassifierName>
I also think it would be best to modify the original jar instead of create
a separate shaded jar.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#995 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Acg-LMGxpkaots9siyyHv8X6QZgmx-Xkks5tGmLvgaJpZM4RPyG3>
.
|
@kpm1985, I guess I am the only one that clicks on the issue number in commit to read the issue ;-) 🥇 |
-Shade libthrift into fluo-core jar
- Shade libthrift into fluo-core jar
@ctubbsii @keith-turner Currently traveling back from Hawaii so my response are going to be delayed. Just a heads up. |
@@ -73,6 +73,10 @@ | |||
<groupId>org.apache.hadoop</groupId> | |||
<artifactId>hadoop-client</artifactId> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.apache.thrift</groupId> | |||
<artifactId>libthrift</artifactId> |
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.
I think this still needs to be <scope>provided</scope>
, or perhaps <optional>true</optional>
in order to prevent transitive dependency resolution by consumers of the fluo-core jar. That is, unless the shade plugin also modifies the POM to exclude libthrift... and I'm not sure it does.
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.
I found that <scope>provided</scope>
would exclude it from the shaded jar so I went with <optional>true</optional>
Also the shaded plugin doesn't modify the original POM
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.
This works, and it probably avoids us having to do anything with the dependency reduced POM... although I would do a comparison with the dependency reduced version manually... just to make sure nothing strange was overlooked.
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.
I didn't see thrift in the reduced POM and it changed the checkstyle plugin config
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 changed a few things... interesting things... but not important things. The reduced POM looks like our POM, so that's good. I'll check the jar contents manually just to do a quick sanity check, but so far, everything looks the way I'd expect.
modules/core/pom.xml
Outdated
<relocations> | ||
<relocation> | ||
<pattern>org.apache.thrift</pattern> | ||
<shadedPattern>org.shaded.thrfit</shadedPattern> |
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.
Typo here... but also, the new package should be a subpackage of org.apache.fluo.core
(or whatever the base package is for this fluo module).
modules/core/pom.xml
Outdated
<includes> | ||
<include>org.apache.fluo.core.*</include> | ||
<include>org.shaded.thrift:*</include> | ||
<include>org.apache.thrift:*</include> |
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.
I'm not sure these includes are correct. The first one looks like a package pattern. The other two look like maven artifacts, but only the groupId. But, there's no such thing as a groupId named org.shaded.thrift
.
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.
I changed them up. The org.apache.fluo.core.*
include is needed so the plugin doesn't pull from the entire project but rather just this module.
The <include>org.shaded.thrift:*</include>
was needed to include the new shaded artifact and the <include>org.apache.thrift:*</include>
was needed for the plugin to relocate thrift.
modules/core/pom.xml
Outdated
<include>org.apache.thrift:*</include> | ||
</includes> | ||
</artifactSet> | ||
<createDependencyReducedPom>false</createDependencyReducedPom> |
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.
If this POM is created, I wonder if it would replace the original POM when installed to the local maven repository or deployed to the remote maven repository. If so, it may be useful to let the plugin create this. See comment above about libthrift dependency being resolved transitively.
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.
I excluded in because I was running into failed maven builds for invalid license header on that generated file.
This seems to be working well. I installed the fluo jars locally and was able to do the following.
@ctubbsii do you think this is ready to merge? |
I ran the following to create the source jars. When I looked at the source jars for Fluo I did not see the shaded source code and the relocation was not done in the source. I am not sure if we want to do anything about this. I think we can open a follow on issue to explore this.
|
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.
I did see one problem when manually inspecting the content of the shaded jar:
It seems to include the LICENSE.txt and NOTICE.txt files from the libthrift. I checked, and there's nothing we need to preserve from these files that isn't already in our own LICENSE and NOTICE file (also included in the shaded jar). So, these just need to be excluded from the libthrift jar when it is shaded in.
I built this locally and verified LICENSE.txt and NOTICE.txt were absent. |
-Shade libthrift into fluo-core jar