[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/1239


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[haohui]

Github user haohui commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-200583518
  
+1


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[revans2]

Github user revans2 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-200538695
  
I am fine with that too. +1 we can make this simpler in the future.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-200529436
  
I'm +1 for merging this. I think moving the jars out of the lib folder is 
the most important part for the 1.0 release. How to package the SQL 
dependencies for the best user experience can be addressed in a follow-up JIRA 
or as simple documentation instructing users what jars need to go where.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[revans2]

Github user revans2 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-22535
  
I don't think that it is actually a fat jar, but I have not run the code so 
I don't really know for sure.

Everything in the JAR comes from the `CompilingClassLoader.getClasses()`


https://github.com/apache/storm/blob/d839d1bf88b855edda344fc548f0701e2a018655/external/sql/storm-sql-core/src/jvm/org/apache/storm/sql/StormSqlImpl.java#L126-L139

Inside CompilingClassLoader the value returned by getClasses is only 
set/added to by the InMemoryFileManager


https://github.com/apache/storm/blob/d839d1bf88b855edda344fc548f0701e2a018655/external/sql/storm-sql-core/src/jvm/org/apache/storm/sql/javac/CompilingClassLoader.java#L184-L201

When it is writing out the compiled .class files.  I don't think it is 
pulling in anything else from the classpath, unless javac somehow has the 
ability to create a fat jar. Which even if that were the case, I don't see 
anything like that being set in the options passed into javac


https://github.com/apache/storm/blob/d839d1bf88b855edda344fc548f0701e2a018655/external/sql/storm-sql-core/src/jvm/org/apache/storm/sql/javac/CompilingClassLoader.java#L141-L168


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[ptgoetz]

Github user ptgoetz commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199989200
  
@revans2 The `storm sql` command (via the `StormSql` class) builds a fat 
topology jar based on the class path, and submits it.

I have some concerns around this approach, as it seems brittle to me. 
Specifically the code that builds the jar does not seem to be 
resource/MANIFEST-aware (unlike the shade plugin). That worries me with respect 
to Hadoop integration (*-site.xml, etc.). There's also the issue with having to 
manually make the kafka jar available.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[revans2]

Github user revans2 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199958977
  
How does the storm sql worker processes get access to the sql jars?  Do the 
workers need access to the jars?  With a name like runtime I would expect that 
it would need access to 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[arunmahadevan]

Github user arunmahadevan commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199733850
  
@abhishekagarwal87 ok makes sense. +1


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[abhishekagarwal87]

Github user abhishekagarwal87 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199670230
  
@arunmahadevan storm-kafka is needed only when kafka source is used in the 
sql. As per the documentation, one approach user can take is to put the 
additional jars in the extlib and then run the sql. Basically storm-kafka 
jar/dependencies are application dependencies which are to be added by user 
manually in classpath. 
There are three categories of jar -
1. storm-core dependencies - under lib - added by storm command line
2. storm-sql-core/runtime dependencies - under 
external/sql/storm-sql-core/runtime (add by `storm sql` command)
3. application level dependencies - To be added in classpath by user - 
storm-kafka jar falls under this category. 



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[arunmahadevan]

Github user arunmahadevan commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199648569
  
@abhishekagarwal87 @harshach the changes looks good and putting the 
storm-sql jars in external/sql/storm-sql-core is consistent with other external 
modules whose jars are under external directory. 

Question :- I don't see the storm-sql-kafka jar added to the `storm sql` 
classpath, so I am not sure how this would  be included in the topology jar or 
in the runtime.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[abhishekagarwal87]

Github user abhishekagarwal87 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199347989
  
@harshach Only the storm-sql-core dependencies are copied into 
external/sql/storm-sql-core. If we put it inside extlib, these jars will be 
added to the worker classpath. The scope of storm-sql-core dependencies should 
be limited to sql command and that purpose won't be served, if they are put in 
extlib. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[abhishekagarwal87]

Github user abhishekagarwal87 commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199120735
  
@haohui Those jars will be present in external/sql/storm-sql-core directory 
and they get added automatically when `storm sql` is run. Below is a sample 
run. Let me know if you see any jar missing

```
 /tmp/apache-storm-2.0.0-SNAPSHOT/bin/storm sql abc abc.sql 

Running: 
/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home/bin/java 
-client -Ddaemon.name= -Dstorm.options= 
-Dstorm.home=/private/tmp/apache-storm-2.0.0-SNAPSHOT 
-Dstorm.log.dir=/private/tmp/apache-storm-2.0.0-SNAPSHOT/logs 
-Djava.library.path=/usr/local/lib:/opt/local/lib:/usr/lib -Dstorm.conf.file= 
-cp 
/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/asm-4.0.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/clojure-1.7.0.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/disruptor-3.3.2.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/kryo-2.21.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/log4j-api-2.1.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/log4j-core-2.1.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/log4j-over-slf4j-1.6.6.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/log4j-slf4j-impl-2.1.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/minlog-1.2.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/reflectasm-1.07-shaded.jar:/private/tmp/apache-storm-2.0
 
.0-SNAPSHOT/lib/servlet-api-2.5.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/slf4j-api-1.7.7.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/storm-core-2.0.0-SNAPSHOT.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/lib/storm-rename-hack-2.0.0-SNAPSHOT.jar:/tmp/apache-storm-2.0.0-SNAPSHOT/conf:/private/tmp/apache-storm-2.0.0-SNAPSHOT/bin:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/calcite-avatica-1.4.0-incubating.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/calcite-core-1.4.0-incubating.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/calcite-linq4j-1.4.0-incubating.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/commons-lang-2.5.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/eigenbase-properties-1.1.5.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/guava-16.0.1.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-co
 
re/jackson-annotations-2.6.3.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/jackson-core-2.6.3.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/jackson-databind-2.6.3.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-core/storm-sql-core-2.0.0-SNAPSHOT.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-runtime/storm-sql-runtime-2.0.0-SNAPSHOT-tests.jar:/private/tmp/apache-storm-2.0.0-SNAPSHOT/external/sql/storm-sql-runtime/storm-sql-runtime-2.0.0-SNAPSHOT.jar
 org.apache.storm.sql.StormSqlRunner abc abc.sql
```


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[haohui]

Github user haohui commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199093496
  
Might need to run some manual tests to validate.

I think there are several jars (e.g., guava) need to be bundled as well. Is 
it a good place to put them in as well?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[harshach]

Github user harshach commented on the pull request:

https://github.com/apache/storm/pull/1239#issuecomment-199088609
  
+1. @haohui can you take a look as well. Do we need to change anything else 
to deploy topologies using storm sql.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1625: Move storm-sql dependencies out of...

[abhishekagarwal87]

GitHub user abhishekagarwal87 opened a pull request:

https://github.com/apache/storm/pull/1239

STORM-1625: Move storm-sql dependencies out of lib folder



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/abhishekagarwal87/storm storm-sql

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/1239.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 #1239


commit e461dc999afff00465bcc998bc53e3539074b0fe
Author: Abhishek Agarwal 
Date:   2016-03-20T10:54:20Z

STORM-1625: Move storm-sql dependencies out of lib folder




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---