[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 No I meant we are publishing a new artifact which has relocated dependencies. You can refer below to see how Flink addressed this. https://github.com/apache/flink-shaded ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 I think "option 2" is good for Storm 2.0 and it may be ok have the users rebuild their topologies that relies on storm-druid and use the relocated package names. With "option 4", users have to ensure that the relocated storm-client jar is present in any storm class paths instead of the regular storm-client jar, so may be tricky. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 4. publish "a" shaded artifact which shades and relocates error-prone artifacts I proposed 4 in discussion thread, and now Flink is leveraging 4. I was worrying about LICENSE issue (because I don't know well) but as Flink goes well with that approach for months, I think it worths to do. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2498 @arunmahadevan I think we are all on the same page here. All I am saying is that this patch by itself does not fix STORM-2881 for 2.x and as such I don't want it to go in as is, we need to come up with a full fix. The 1.x patch looks good to me and in this case I would be fine with merging it in without a full 2.x fix. The choices I see to fix 2.x are 1) shade curator in storm-client, 2) shade curator in storm-druid, or 3) we change tranquility to use an up to date version of curator. Shading has lots of problems. Newer versions of maven don't support it properly and it makes development with an IDE a real pain because the IDE does not know about it. These are the reasons that we wanted to go away from using it, but I think getting working code trumps all of them. If you do want to do shading again, I think I have some ideas that might mitigate some of the IDE issues, but I personally think that choice 2 is the best long term fix. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 @revans2 , the latest version of tranquility-core available is 0.8.2 which has this curator 2.6.0 transitive dependency. I think the storm-druid unit tests would use the 2.6.0 version of curator since the version is explicitly mentioned in the storm-druid pom. If we decide to not relocate the storm's curator dependencies, storm-druid with this patch will have a curator version conflict at runtime and may fail. The other option could be to shade storm-druid and relocate its curator dependencies, but it may break the existing topologies using storm-druid and they would need to use the relocated package names. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2498 This patch is problematic without shading. With this the unit tests will run with a different version of curator from how it will run in production. So it ends up being a NOOP with masking errors in the unit tests. If we want to do shading we can, but honestly why is druid using a three and a half year old version of curator, and what is the impact of upgrading? ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 Maybe we could file a blocker issue for Storm 2.0.0 and allow this patch. Filed https://issues.apache.org/jira/browse/STORM-2882 for that. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 It doesn't represent we decided to not relocate. We just didn't decide it, and it should be done within releasing 2.0.0. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 https://mail-archives.apache.org/mod_mbox/incubator-storm-dev/201703.mbox/%3ccaf5108gjjqzsyywcp99bgpgec7tufe-tbt9fi0m78ah8rkm...@mail.gmail.com%3E You can follow up the discussion to see why I removed relocation for now. We are splitting storm-core module to several parts, and if we relocate something and refer it within the project, it should be much painful. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 @HeartSaVioR , any reason why we dont relocate in 2.0.0 vs 1.x ? If not users would hit issues with conflicting versions (like curator). ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2498 In master branch we don't relocate any dependencies so it will make conflict anyway. Looks like this case represents that we should have a solution for 2.0.0 sooner. ---
[GitHub] storm issue #2498: STORM-2881: Explicitly specify the curator dependencies i...
Github user arunmahadevan commented on the issue: https://github.com/apache/storm/pull/2498 ping @omkreddy , @HeartSaVioR ---
