[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread srdo
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2698 Looks great. +1. ---

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread ptgoetz
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread srdo
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread srdo
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

2018-06-06 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-06 Thread srdo
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-05 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-05 Thread srdo
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-06-05 Thread revans2
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

2018-06-04 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-05-30 Thread revans2
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

[GitHub] storm issue #2698: STORM-2882: shade storm-client dependencies

2018-05-30 Thread revans2
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