[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 Dependencies for the client in 1.2.1 asm clojure - removed in 2.x disruptor - removed by this patch gmetric4j kryo log4j2 log4j1.2-api metrics-core metrics-ganglia metrics-graphite minlog objeneses reflectasm ring-cors - removed in 2.x servlet-api-2.5 - removed by this patch slf4j storm-core - removed in 2.x storm-rename-hack - removed in 2.x The only dependency on the storm client classpath not in 1.2.1 is org.acplt:oncrpc:jar:1.0.7 because it was pulled in by a newer version of io.dropwizard.metrics:metrics-ganglia. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 Shading here is different from how it is in 1.x. For this one there is a separate package that creates a shaded uber jar. The code inside storm-client and storm-server that need to use it will call the shaded APIs directly. The upside is that IDEs work grate because they are operating on the shaded APIs and don't have to worry about how shading works. The downside is that doing a full build is now a 2 step process. This is because of a "bug" in maven/shade were shading in a multi-module project results in down stream builds not seeing the shaded dependencies. I cannot really combine them into a single build step because the version of maven with this bug in it is now standard in most places, so our "fix" for 1.x is not really valid any more. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 @HeartSaVioR I have addressed the other review comments, but I could not find a place in the documentation for instructions on how to do a release. I agree that we want to publish the shaded-deps too, or clients will not be able to find their dependencies. Just not sure where that is documented, or what scripts there are that do it, if any. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 I addressed everything except for jaxb. Because it is for java9 I will download a java9 JDK and work on that to make sure it works there too, so it might be a little longer. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 Just so you don't spend too much time on it, neither HBase nor Cassandra work with Java 9 right now. I skipped those modules when testing it last year. Also I didn't get farther than building and running the tests with Java 9, so I can't say whether it works beyond that. At the time I was hoping to be able to upgrade HBase and Cassandra before too long and do more tests after that, but they are still working on Java 9 support. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 All I did was build/run the unit tests and this time I was able to get everything to pass. Oddly I was able to compile all of the code without jaxb-api but I could not run the tests... I decided not to shade the API because I am not sure where the implementation is, and I would need to shade both. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 Thanks for addressing the comments. Looks good to me. I think it makes sense to add shaded-deps as a module to the root pom though. It will likely address the release issue, and means we don't have to manually keep its version number in sync with the rest of Storm. Is there a reason to not have it listed in the root? ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 @srdo I can add shaded-deps to the root pom, but it is going to potentially mask a lot of build issues, and is going to make IDEs confused. maven-3.2.6 and above added in a cache for dependencies in the pom.xml. The shade plugin tries to modify those dependencies on the fly and the maven cache wins. https://issues.apache.org/jira/browse/MSHADE-206 What this means is that if shaded-deps is a part of the root pom.xml, all of the code built by maven is going to have both the shaded and the non-shaded version on the classpath when it compiles and when unit tests run, but only the shaded version will be on the classpath in production. Similarly for IDEs that don't understand the shading plugin they will get confused and will likely not see the shaded dependencies so all the code I changed will start to show up as errors, while anyone who uses it will write code against the non-shaded deps, which because of MSHADE-206, will compile under maven, but won't run in production. In older releases we told everyone to use an older version of maven that is not impacted by this, but that is getting harder and harder to do, especially with recent security disclosures. If you really want me to I can add it in under a profile that would be on by default, but we could turn off when we do the check-in builds and when being imported into an IDE. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 Thanks @ptgoetz ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/2698 +1 @srdo You asked what the maven release commands were. They are: ``` mvn release:prepare -P dist mvn release:perform -P dist ``` Note that the process modifies the pom files, checks in changes, tags, and pushes to ASF git. So if you want to test the release process you will need to revert those changes. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 @revans2 If I'm understanding the problem correctly, the bug is that if shaded-deps is in the same reactor as the other modules, Maven will read the "normal" pom, rather than the dependency reduced pom, and put all the shaded-deps dependencies onto the classpath of the other modules? Would it work to make all the dependencies declared in shaded-deps optional (i.e. non-transitive)? ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 @srdo It might work to make them optional. I am not a maven expert when it comes to that..., or really just about anything with maven beyond setting up a basic build. I am happy to try it out, as it is a real pain. I will also try out using a profile to control when/how it is built, so hold off checking any of this in until I have tested them. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2698 @srdo your suggestion to mark the dependencies as optional in the shaded pom was a great one. I have it working now where shaded-deps is built by default, maven does not include any of the unwanted dependencies on the classpath while building or testing, and when setting up and IDE there is a profile that can be used to disable shaded-deps so the IDE is happy. What is more IntelliJ has that off by default so if you just import storm it will do the right thing out of the box. I also updated the travis scripts to explicitly build/install shaded-deps first and then run the rest of the build with shaded-deps off, so it we know nothing is sneaking in there on accident. ---
[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 Looks great. +1. ---
