You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by sakanaou <gi...@git.apache.org> on 2016/06/10 10:41:12 UTC
[GitHub] storm pull request #1480: STORM-1881: Create fat-jar of storm-redis which co...
GitHub user sakanaou opened a pull request:
https://github.com/apache/storm/pull/1480
STORM-1881: Create fat-jar of storm-redis which contains needed dependencies
`storm-redis` was packaged in a way that it was not immediately usable when put into `extlib` folder. `ClassNotFoundException`s would occur.
Via `maven-shade-plugin` a "fat"-jar is created so all depending code is packaged in.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sakanaou/storm redis-shadedjar
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1480.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1480
----
commit b6f89e7a5f63a59d942ecbc63b7ea6711824946e
Author: Daniel Klessing <da...@iqser.com>
Date: 2016-06-10T10:36:53Z
STORM-1881: Create fat-jar of storm-redis which contains needed dependencies
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1480: STORM-1881: Create fat-jar of storm-redis which co...
Posted by sakanaou <gi...@git.apache.org>.
Github user sakanaou closed the pull request at:
https://github.com/apache/storm/pull/1480
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
We're shading the libraries which you're changing scope to `provided` in storm-core, so it will not be provided at runtime.
Since storm-redis depends on Guava, placing uber storm-redis jar to extlib incurs dependency issue in whole topologies in cluster. If we still really want to place this to extlib, we may need to consider shading guava and others for storm-redis module.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by sakanaou <gi...@git.apache.org>.
Github user sakanaou commented on the issue:
https://github.com/apache/storm/pull/1480
Closed as requested due to STORM-2016.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by sakanaou <gi...@git.apache.org>.
Github user sakanaou commented on the issue:
https://github.com/apache/storm/pull/1480
@HeartSaVioR: `storm-core/pom.xml` includes a `maven-shade-plugin` step that lists both guava and jackson as artifactSet-includes without relocations. Why should they then not be provided via storm-core? I have successfully used storm-redis without placing guava or jackson explicitly in the classpath so they must be present already. From my understanding they are provided by the "fat" storm-core jar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
Getting rid of Guava seems easy since storm-redis uses Precondition, ImmutableMap, Maps, Lists.
Getting rid of Jackson seems not easy since we take StateConfig to JSON format so we need JSON parser anyway.
It might need to explain current status and discuss how to resolve it from dev@.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1480
guava dependency is relocated in storm-core.
https://github.com/apache/storm/blob/2a435b0289d43494eb2f834e94ed61c175f9eea6/storm-core/pom.xml#L593
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1480
If you use already relocated classes, you make storm-redis module dependent on a particular version of storm-core. relocation pattern is subject to changes in storm-core (since it is internal) and if that happens, your module stops being compatible with storm-core. So you should avoid using already relocated classes in storm-core.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
Sorry for long delay, say, 1 month.
I've met similar issue on Storm SQL, so I've initiate discussion to dev@ list.
Along with discussion, I'm trying to see how Spark handles dependencies and adding them to executor. I guess I understand how it works, but I didn't play with how to resolve transitive dependencies and download them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
Let me have a look. I guess storm-redis doesn't need to depend on other than storm-core and Jedis.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1480
just curious, how much is the size of fat jar?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by sakanaou <gi...@git.apache.org>.
Github user sakanaou commented on the issue:
https://github.com/apache/storm/pull/1480
@abhishekagarwal87: Ok, didn't spot that. Was only looking for "guava" relocations.
So, the dependency could be additionally included relocated into storm-redis. Or is it possible to use the already relocated package which is provided via storm-core? This would save some resources. I'm not very experienced with the `maven-shade-plugin` yet, unfortunately.
What is preferred?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
@sakanaou
Since STORM-2016 is merged into master and 1.x-branch, so I think we can close this.
Could you please doing it? Thanks in advance!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by sakanaou <gi...@git.apache.org>.
Github user sakanaou commented on the issue:
https://github.com/apache/storm/pull/1480
@abhishekagarwal87: Original jar is 65K, shaded jar is 709K. guava and jackson are already contained in storm-core so those can be ignored, luckily.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1480
Thanks @sakanaou
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1480: STORM-1881: Create fat-jar of storm-redis which contains ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1480
STORM-2016 is on reviewing, and it should help this get resolved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---