You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2016/08/05 04:29:54 UTC
[GitHub] storm pull request #1608: STORM-2016 Topology submission improvement: suppor...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1608
STORM-2016 Topology submission improvement: support adding local jars and maven artifacts on submission (1.x)
* JIRA issue: http://issues.apache.org/jira/browse/STORM-2016
* design doc: https://cwiki.apache.org/confluence/display/STORM/A.+Design+doc%3A+adding+jars+and+maven+artifacts+at+submission
* discussion thread: http://mail-archives.apache.org/mod_mbox/storm-dev/201608.mbox/%3CCAF5108i9+tJaNZ0LgRkTMkVQEL7F+53k9uyzxcT6zhSU6OHx9Q@mail.gmail.com%3E
----
* bin/storm now supports "--jars" and "--packages" options
* it's only effective with "storm jar" and "storm sql"
* introduce new module: storm-submit to help resolving dependencies with handling transitive dependencies
* StormSubmitter will upload dependencies to BlobStore when submitting topology
* Supervisor will download dependencies from BlobStore when such topology is assigned
* Supervisor will launch workers with adding downloaded dependencies to worker classpath
TODO
* documentation - [ ]
* craft pull request against master branch - [ ]
Btw, it might be better to place some modules out of external since 'external' has most of non storm-core modules and flux, sql, storm-submit, storm-kafka-monitor are not kind of connectors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2016-1.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1608.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 #1608
----
commit e027652eb30f3c3b3a549a24d4105f9dd1e8e45e
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2016-08-04T00:52:45Z
STORM-2016 Topology submission improvement: support adding local jars and maven artifacts on submission
* bin/storm now supports "--jars" and "--packages" options
** it's only effective with "storm jar" and "storm sql"
* introduce new module: storm-submit to help resolving dependencies with handling transitive dependencies
* StormSubmitter will upload dependencies to BlobStore when submitting topology
* Supervisor will download dependencies from BlobStore when such topology is assigned
* Supervisor will launch workers with adding downloaded dependencies to worker classpath
----
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74032406
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
--- End diff --
Personally I prefer `!`, but if escaping matters I think `^` is also fine. I'll address this.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73822736
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
+ jars_list.extend(curr.pop().split(','))
+ elif token == "--artifacts":
--- End diff --
No, the term `artifact` is also used from Ivy, too.
http://ant.apache.org/ivy/history/latest-milestone/terminology.html#artifact
I think term `dependencies` is closer to jars + artifacts since jars will be also added to classpath.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Porting this to master forces me to resolve huge conflict or modify the code manually. We shouldn't leave progress of Java port as it is.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74014016
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
--- End diff --
another common character for **not** is `^`
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang A bit confusing so would like to double check on this. Do you succeed to resolve 1.1.1-SNAPSHOT after `mvn clean install`? Or you're still failing?
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang 1.1.1-SNAPSHOT is not officially released (we don't release SNAPSHOT officially) so you need to build entire Storm code with `mvn clean install` before doing that. We can change the example to 1.1.0 or so after the feature is released to official.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74539478
--- Diff: bin/storm.py ---
@@ -232,44 +271,91 @@ def jar(jarfile, klass, *args):
The process is configured so that StormSubmitter
(http://storm.apache.org/releases/current/javadocs/org/apache/storm/StormSubmitter.html)
will upload the jar at topology-jar-path when the topology is submitted.
+
+ When you want to ship other jars which is not included to application jar, you can pass them to --jars option with comma-separated string.
+ For example, --jars "your-local-jar.jar,your-local-jar2.jar" will load your-local-jar.jar and your-local-jar2.jar.
+ And when you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
+ You can also exclude some dependencies like what you're doing in maven pom.
+ Please add exclusion artifacts with '^' separated string after the artifact.
+ For example, --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12" will load jedis and kafka artifact and all of transitive dependencies but exclude slf4j-log4j12 from kafka.
+
+ Complete example of both options is here: `./bin/storm jar example/storm-starter/storm-starter-topologies-*.jar org.apache.storm.starter.RollingTopWords blobstore-remote2 remote --jars "./external/storm-redis/storm-redis-1.1.0.jar,./external/storm-kafka/storm-kafka-1.1.0.jar" --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12"`
+
+ When you pass jars and/or artifacts options, StormSubmitter will upload them when the topology is submitted, and they will be included to classpath of both the process which runs the class, and also workers for that topology.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
transform_class = confvalue("client.jartransformer.class", [CLUSTER_CONF_DIR])
if (transform_class != None and transform_class != "nil"):
tmpjar = os.path.join(tempfile.gettempdir(), uuid.uuid1().hex+".jar")
exec_storm_class("org.apache.storm.daemon.ClientJarTransformerRunner", args=[transform_class, jarfile, tmpjar], fork=True, daemon=False)
+ extra_jars = [tmpjar, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
topology_runner_exit_code = exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
fork=True,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
os.remove(tmpjar)
sys.exit(topology_runner_exit_code)
else:
+ extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
def sql(sql_file, topology_name):
"""Syntax: [storm sql sql-file topology-name]
Compiles the SQL statements into a Trident topology and submits it to Storm.
+
+ --jars and --artifacts options available for jar are also applied to sql command.
+ Please refer "help jar" to see how to use --jars and --artifacts options.
+ You normally want to pass these options since you need to set data source to your sql which is an external storage in many cases.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
+ sql_core_jars = get_jars_full(STORM_DIR + "/external/sql/storm-sql-core")
--- End diff --
@HeartSaVioR sounds good to me. We can address this in another JIRA.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
+1
@HeartSaVioR Nice work.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74005642
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -233,6 +236,23 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
if(topologyNameExists(conf, name, asUser)) {
throw new RuntimeException("Topology with name `" + name + "` already exists on cluster");
}
+
+ // Dependency uploading only makes sense for distributed mode
+ List<String> jarsBlobKeys;
+ List<String> artifactsBlobKeys;
+
+ DependencyUploader uploader = new DependencyUploader();
+ try {
+ uploader.init();
+ jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
+ artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
--- End diff --
We can do that for jarsBlobKeys but can't do that for artifactsBlobKeys since we share artifact blobs and we skip uploads if there're same artifact uploaded before.
I'll address this for jarsBlobKeys. Thanks!
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74002489
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
--- End diff --
'!' occasionally represents 'not', so I think it would be easier to be recognized as excluded. Do we feel similar from '~'?
Refer to http://www.gnu.org/software/bash/manual/bashref.html#Quoting we should avoid to use '$', '`', '\', '"', '!', '@', '*' in double quotes if we want to avoid escape.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Changed module name as suggestion from pull request on master branch.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74365965
--- Diff: bin/storm.py ---
@@ -232,44 +271,91 @@ def jar(jarfile, klass, *args):
The process is configured so that StormSubmitter
(http://storm.apache.org/releases/current/javadocs/org/apache/storm/StormSubmitter.html)
will upload the jar at topology-jar-path when the topology is submitted.
+
+ When you want to ship other jars which is not included to application jar, you can pass them to --jars option with comma-separated string.
+ For example, --jars "your-local-jar.jar,your-local-jar2.jar" will load your-local-jar.jar and your-local-jar2.jar.
+ And when you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
+ You can also exclude some dependencies like what you're doing in maven pom.
+ Please add exclusion artifacts with '^' separated string after the artifact.
+ For example, --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12" will load jedis and kafka artifact and all of transitive dependencies but exclude slf4j-log4j12 from kafka.
+
+ Complete example of both options is here: `./bin/storm jar example/storm-starter/storm-starter-topologies-*.jar org.apache.storm.starter.RollingTopWords blobstore-remote2 remote --jars "./external/storm-redis/storm-redis-1.1.0.jar,./external/storm-kafka/storm-kafka-1.1.0.jar" --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12"`
+
+ When you pass jars and/or artifacts options, StormSubmitter will upload them when the topology is submitted, and they will be included to classpath of both the process which runs the class, and also workers for that topology.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
transform_class = confvalue("client.jartransformer.class", [CLUSTER_CONF_DIR])
if (transform_class != None and transform_class != "nil"):
tmpjar = os.path.join(tempfile.gettempdir(), uuid.uuid1().hex+".jar")
exec_storm_class("org.apache.storm.daemon.ClientJarTransformerRunner", args=[transform_class, jarfile, tmpjar], fork=True, daemon=False)
+ extra_jars = [tmpjar, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
topology_runner_exit_code = exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
fork=True,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
os.remove(tmpjar)
sys.exit(topology_runner_exit_code)
else:
+ extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
def sql(sql_file, topology_name):
"""Syntax: [storm sql sql-file topology-name]
Compiles the SQL statements into a Trident topology and submits it to Storm.
+
+ --jars and --artifacts options available for jar are also applied to sql command.
+ Please refer "help jar" to see how to use --jars and --artifacts options.
+ You normally want to pass these options since you need to set data source to your sql which is an external storage in many cases.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
+ sql_core_jars = get_jars_full(STORM_DIR + "/external/sql/storm-sql-core")
--- End diff --
making this hardcoded path to external code doesn't sound right. we should make sql as top-level directory
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR The code looks good (non-binding). I tried running `storm sql` but ended up with timeout exceptions
```
Exception in thread "main" java.lang.RuntimeException: org.sonatype.aether.resolution.DependencyResolutionException: Failed to collect dependencies for org.apache.storm:storm-sql-kafka:jar:1.1.1-SNAPSHOT (compile)
at org.apache.storm.submit.command.DependencyResolverMain.main(DependencyResolverMain.java:63)
Caused by: org.sonatype.aether.resolution.DependencyResolutionException: Failed to collect dependencies for org.apache.storm:storm-sql-kafka:jar:1.1.1-SNAPSHOT (compile)
at org.sonatype.aether.impl.internal.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:371)
at org.apache.storm.submit.dependency.DependencyResolver.resolve(DependencyResolver.java:67)
at org.apache.storm.submit.command.DependencyResolverMain.main(DependencyResolverMain.java:52)
Caused by: org.sonatype.aether.collection.DependencyCollectionException: Failed to collect dependencies for org.apache.storm:storm-sql-kafka:jar:1.1.1-SNAPSHOT (compile)
at org.sonatype.aether.impl.internal.DefaultDependencyCollector.collectDependencies(DefaultDependencyCollector.java:258)
at org.sonatype.aether.impl.internal.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:333)
... 2 more
Caused by: org.sonatype.aether.resolution.ArtifactDescriptorException: Failed to read artifact descriptor for org.apache.kafka:kafka_2.10:jar:0.8.2.2\
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:282)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.readArtifactDescriptor(DefaultArtifactDescriptorReader.java:172)
at org.sonatype.aether.impl.internal.DefaultDependencyCollector.process(DefaultDependencyCollector.java:412)
at org.sonatype.aether.impl.internal.DefaultDependencyCollector.collectDependencies(DefaultDependencyCollector.java:240)
... 3 more
Caused by: org.sonatype.aether.resolution.ArtifactResolutionException: Could not transfer artifact org.apache.kafka:kafka_2.10:pom:0.8.2.2\ from/to central (http://repo1.maven.org/maven2/): Error transferring file: Connection timed out
at org.sonatype.aether.impl.internal.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:537)
at org.sonatype.aether.impl.internal.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:216)
at org.sonatype.aether.impl.internal.DefaultArtifactResolver.resolveArtifact(DefaultArtifactResolver.java:193)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:267)
... 6 more
Caused by: org.sonatype.aether.transfer.ArtifactTransferException: Could not transfer artifact org.apache.kafka:kafka_2.10:pom:0.8.2.2\ from/to central (http://repo1.maven.org/maven2/): Error transferring file: Connection timed out
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector$4.wrap(WagonRepositoryConnector.java:975)
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector$4.wrap(WagonRepositoryConnector.java:966)
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector$GetTask.flush(WagonRepositoryConnector.java:707)
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector$GetTask.flush(WagonRepositoryConnector.java:701)
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector.get(WagonRepositoryConnector.java:452)
at org.sonatype.aether.impl.internal.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:456)
... 9 more
Caused by: org.apache.maven.wagon.TransferFailedException: Error transferring file: Connection timed out
at org.apache.maven.wagon.providers.http.LightweightHttpWagon.fillInputData(LightweightHttpWagon.java:143)
at org.apache.maven.wagon.StreamWagon.getInputStream(StreamWagon.java:116)
at org.apache.maven.wagon.StreamWagon.getIfNewer(StreamWagon.java:88)
at org.apache.maven.wagon.StreamWagon.get(StreamWagon.java:61)
at org.sonatype.aether.connector.wagon.WagonRepositoryConnector$GetTask.run(WagonRepositoryConnector.java:615)
at org.sonatype.aether.util.concurrency.RunnableErrorForwarder$1.run(RunnableErrorForwarder.java:60)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.net.ConnectException: Connection timed out
at java.net.PlainSocketImpl.socketConnect(Native Method)
at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
at java.net.Socket.connect(Socket.java:589)
at java.net.Socket.connect(Socket.java:538)
at sun.net.NetworkClient.doConnect(NetworkClient.java:180)
at sun.net.www.http.HttpClient.openServer(HttpClient.java:432)
at sun.net.www.http.HttpClient.openServer(HttpClient.java:527)
at sun.net.www.http.HttpClient.<init>(HttpClient.java:211)
at sun.net.www.http.HttpClient.New(HttpClient.java:308)
at sun.net.www.http.HttpClient.New(HttpClient.java:326)
at sun.net.www.protocol.http.HttpURLConnection.getNewHttpClient(HttpURLConnection.java:1169)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1105)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:999)
at sun.net.www.protocol.http.HttpURLConnection.connect(HttpURLConnection.java:933)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1513)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1441)
at java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
at org.apache.maven.wagon.providers.http.LightweightHttpWagon.fillInputData(LightweightHttpWagon.java:115)
... 8 more
```
I may have some problems connecting to maven central but why the resolver didn't look at my local repo which did contain the required 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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73823982
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
That is not mandatory, but it only makes sense when that jar is not presented neither system provided nor included to application jar. If system or application jar provides that, that will be used. This is similar to precedence of classloading between system and application 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 pull request #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73823221
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
--- End diff --
It would be better to add its usage to comment doc of `jar` and `sql` since print_usage(jar) will print out the usage of jar, and so on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Builds for PR against 1.x branch is failing and I fixed that from #1609. I'll rebase once #1609 is merged to 1.x-branch.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73823457
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
Could we have some examples here ?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73985493
--- Diff: storm-core/src/clj/org/apache/storm/config.clj ---
@@ -141,6 +141,10 @@
[conf stormconf-path]
(merge conf (clojurify-structure (Utils/fromCompressedJsonConf (FileUtils/readFileToByteArray (File. stormconf-path))))))
+(defn read-supervisor-storm-code-given-path
--- End diff --
Great point! Will address.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Fixed build failure on supervisor_test.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd @abhishekagarwal87
Please review again when you get some time. I'll squash the commits and go on porting this to master branch once some of us give +1 for this.
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Done. Fixed things after receiving +1 are here:
1. nimbus.clj: just log exception instead of crashing nimbus when blob-rm-dependency-jars-in-topology
2. DependencyPropertiesParser.java: check --jars parameter string is empty, and treat it as empty list (also add new unit test testing this change)
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74001827
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/command/DependencyResolverMain.java ---
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.submit.command;
+
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.apache.storm.submit.dependency.AetherUtils;
+import org.apache.storm.submit.dependency.DependencyResolver;
+import org.json.simple.JSONValue;
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.resolution.ArtifactResult;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+public class DependencyResolverMain {
+
+ public static void main(String[] args) {
+ if (args.length < 1) {
+ throw new IllegalArgumentException("packages must be presented.");
--- End diff --
Great finding. Will address.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73704312
--- Diff: bin/storm.py ---
@@ -154,6 +157,39 @@ def confvalue(name, extrapaths, daemon=True):
return " ".join(tokens[1:])
return ""
+def resolve_dependencies(packages):
--- End diff --
Good point. Will address.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74367004
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
+ jars_list.extend(curr.pop().split(','))
+ elif token == "--artifacts":
--- End diff --
This is already documented for `jar` so you can refer `help jar` to see explanation. I've also documented to doc page.
The intention behind picking `artifacts` is that, I picked `packages` first since this is what Spark already used, but realized why they use `packages` as term is that they maintain spark-packages. So `artifacts` is the alternatives for me.
Btw, the term `artifact` is used by Maven and Ivy, and Aether also uses `Artifact` class to represent this.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
The exception is caused by the escaping "\" character previously. Things are working now after I updated to the latest version.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@harshach Sure, I'll wait for the review during this week. Btw, Actual modification is around (or less than) 1000 lines, not that much.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74029648
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -238,39 +239,24 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
// Dependency uploading only makes sense for distributed mode
- List<String> jarsBlobKeys;
+ List<String> jarsBlobKeys = Collections.emptyList();
List<String> artifactsBlobKeys;
DependencyUploader uploader = new DependencyUploader();
try {
uploader.init();
jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
+ setDependencyBlobsToTopology(topology, jarsBlobKeys, artifactsBlobKeys);
+ submitTopologyInDistributeMode(name, topology, opts, progressListener, asUser, conf, serConf);
+ } catch (Throwable e) {
+ // remove uploaded jars blobs, not artifacts since they're shared across the cluster
+ uploader.deleteBlobs(jarsBlobKeys);
--- End diff --
@HeartSaVioR Is not there a possibility that the topology is submitted successfully on nimbus but client receives thrift error because of some timeout or connection error?
My preference is to remove those topologies only when we know definitely that the topology submission is failed.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73691111
--- Diff: bin/storm.py ---
@@ -154,6 +157,39 @@ def confvalue(name, extrapaths, daemon=True):
return " ".join(tokens[1:])
return ""
+def resolve_dependencies(packages):
--- End diff --
nit: It would be nice to have a short circuit here so if there are no dependencies to resolve we don't have to call into another java application.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74005045
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -233,6 +236,23 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
if(topologyNameExists(conf, name, asUser)) {
throw new RuntimeException("Topology with name `" + name + "` already exists on cluster");
}
+
+ // Dependency uploading only makes sense for distributed mode
+ List<String> jarsBlobKeys;
+ List<String> artifactsBlobKeys;
+
+ DependencyUploader uploader = new DependencyUploader();
+ try {
+ uploader.init();
+ jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
+ artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
--- End diff --
Both `jarsBlobKeys` and `artifactsBlobKeys` should be removed from blob store when topology submission is failed.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73985944
--- Diff: storm-core/src/clj/org/apache/storm/config.clj ---
@@ -141,6 +141,10 @@
[conf stormconf-path]
(merge conf (clojurify-structure (Utils/fromCompressedJsonConf (FileUtils/readFileToByteArray (File. stormconf-path))))))
+(defn read-supervisor-storm-code-given-path
--- End diff --
Addressed.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR can you give it 2 days given that its a big patch. Thanks.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73841599
--- Diff: storm-core/src/clj/org/apache/storm/daemon/supervisor.clj ---
@@ -893,29 +891,61 @@
(let [rsrc-file-path (File. (.getFilePath local-rsrc))
key-name (.getName rsrc-file-path)
blob-symlink-target-name (.getName (File. (.getCurrentSymlinkPath local-rsrc)))
- symlink-name (get-blob-localname (get blobstore-map key-name) key-name)]
+ symlink-name (fn-symlink-name blobs key-name)]
(create-symlink! tmproot (.getParent rsrc-file-path) symlink-name
- blob-symlink-target-name))))
+ blob-symlink-target-name))))
(catch AuthorizationException authExp
(log-error authExp))
(catch KeyNotFoundException knf
(log-error knf))))))
+(defn download-blobs-in-blobstore-map-for-topology!
+ "Download all blobs listed in the topology configuration for a given topology."
+ [conf stormconf-path localizer tmproot]
+ (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
+ blobstore-map (storm-conf TOPOLOGY-BLOBSTORE-MAP)]
+ (download-blobs-for-topology! conf storm-conf localizer tmproot blobstore-map
+ (fn [blobs] (blobstore-map-to-localresources blobs))
+ (fn [blobs key-name] (get-blob-localname (get blobs key-name) key-name)))))
+
+(defn download-dependencies-for-topology!
+ "Download all dependencies blobs listed in the topology configuration for a given topology."
+ [conf stormconf-path stormcode-path localizer tmproot]
+ (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
+ storm-code (read-supervisor-storm-code-given-path stormcode-path)
+ dependencies (concat (.get_dependency_jars ^StormTopology storm-code)
+ (.get_dependency_artifacts ^StormTopology storm-code))]
+ (download-blobs-for-topology! conf storm-conf localizer tmproot dependencies
+ (fn [blobs] (map #(LocalResource. % false) blobs))
+ (fn [_ key-name] key-name))))
+
(defn get-blob-file-names
[blobstore-map]
(if blobstore-map
(for [[k, data] blobstore-map]
(get-blob-localname data k))))
(defn download-blobs-for-topology-succeed?
+ [target-dir file-names]
+ (if-not (empty? file-names)
+ (every? #(Utils/checkFileExists target-dir %) file-names)
+ true))
+
+(defn download-blobs-in-blobstore-map-for-topology-succeed?
--- End diff --
It seems these methods could be private ?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73825149
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
It's not clear whether system jar or application jar will get loaded and may create some surprise if the uploaded jar doesn't work. Maybe some warnings somewhere.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@abhishekagarwal87
Thanks, I took a look and your modification is simpler.
Btw, I was thinking same approach, but there're some cases to handle while assembling jars (for example, the content of schema files for Spring modules should be all added into one file) so it doesn't work that cases. That's why I choose this approach. It's already used widely by Spark users indeed.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74027328
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -238,39 +239,24 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
// Dependency uploading only makes sense for distributed mode
- List<String> jarsBlobKeys;
+ List<String> jarsBlobKeys = Collections.emptyList();
List<String> artifactsBlobKeys;
DependencyUploader uploader = new DependencyUploader();
try {
uploader.init();
jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
+ setDependencyBlobsToTopology(topology, jarsBlobKeys, artifactsBlobKeys);
+ submitTopologyInDistributeMode(name, topology, opts, progressListener, asUser, conf, serConf);
+ } catch (Throwable e) {
+ // remove uploaded jars blobs, not artifacts since they're shared across the cluster
+ uploader.deleteBlobs(jarsBlobKeys);
--- End diff --
@satishd
In fact, submitTopologyWithOpts() can also throw AuthorizationException and org.apache.thrift.TException, too.
Let's back to flow of logic.
Please note that we didn't catch Throwable (some known Exceptions and also RuntimeException) and just pass to caller. And there's also no return for Nimbus.Client.submitTopology(), which indicates that unless we get TException or other Exceptions it should be treated as succeed. (If not we can't guarantee topology deploy is succeed.)
I think this assumption is right for now, and assumption should be right unless we add some operations after calling submitTopologyInDistributeMode() or adding some operations after logging "Finished submitting topology".
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74002305
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
+ Collection<Exclusion> exclusions = new ArrayList<>();
+ for (int idx = 1 ; idx < dependencyAndExclusions.size() ; idx++) {
+ exclusions.add(AetherUtils.createExclusion(dependencyAndExclusions.get(idx)));
+ }
+
+ Artifact artifact = new DefaultArtifact(dependencyAndExclusions.get(0));
+ return new Dependency(artifact, JavaScopes.COMPILE, false, exclusions);
+ }
+
+ public static Exclusion createExclusion(String exclusionString) {
+ String[] parts = exclusionString.split(":");
+
+ // length of parts should be greater than 0
+ String groupId = parts[0];
+
+ String artifactId = "*";
+ String classifier = "*";
+ String extension = "*";
+
+ int len = parts.length;
+ if (len > 1) {
+ artifactId = parts[1];
+ }
+ if (len > 2) {
+ classifier = parts[2];
+ }
+ if (len > 3) {
+ extension = parts[3];
+ }
+
+ return new Exclusion(groupId, artifactId, classifier, extension);
+ }
+
+ public static String artifactToString(Artifact artifact) {
+ StringBuilder buffer = new StringBuilder(128);
+ buffer.append(artifact.getGroupId());
+ buffer.append(':').append(artifact.getArtifactId());
+ buffer.append(':').append(artifact.getExtension());
+ if(artifact.getClassifier().length() > 0) {
--- End diff --
nit: space here
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74023201
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -238,39 +239,24 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
// Dependency uploading only makes sense for distributed mode
- List<String> jarsBlobKeys;
+ List<String> jarsBlobKeys = Collections.emptyList();
List<String> artifactsBlobKeys;
DependencyUploader uploader = new DependencyUploader();
try {
uploader.init();
jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
+ setDependencyBlobsToTopology(topology, jarsBlobKeys, artifactsBlobKeys);
+ submitTopologyInDistributeMode(name, topology, opts, progressListener, asUser, conf, serConf);
+ } catch (Throwable e) {
+ // remove uploaded jars blobs, not artifacts since they're shared across the cluster
+ uploader.deleteBlobs(jarsBlobKeys);
--- End diff --
InvalidTopologyException and AlreadyAliveException definitely indicates that the topology is not successfully deployed and respective jars can be removed from blob store.
@HeartSaVioR
But this code assumes that any Exception thrown here indicates that the submitted topolgy is not deployed successfully. I am not sure whether that assumption is always right.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang Thanks, I addressed your latest comments from https://github.com/apache/storm/pull/1608/commits/a9f4886fd2f093c7a4450b55ef809fc9826c820e
Not sure we have better representation than '!' without conflicting bash special char and also not used char for maven artifact.
@satishd Thanks, I addressed your last comment from https://github.com/apache/storm/pull/1608/commits/17cb2dd333205578bed8f8af8a51072b18daf32d
For custom ACL for jars blobs I'd rather see possible issues first when we set ACL to blobs. If it's no issue at all and we agree that there could be a case of this I'll file an issue.
Please take a look again. Thanks!
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang OK great. Thanks for the update.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74005317
--- Diff: storm-core/src/jvm/org/apache/storm/dependency/DependencyUploader.java ---
@@ -0,0 +1,166 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.dependency;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.commons.lang.StringUtils;
+import org.apache.storm.blobstore.AtomicOutputStream;
+import org.apache.storm.blobstore.BlobStoreUtils;
+import org.apache.storm.blobstore.ClientBlobStore;
+import org.apache.storm.generated.AccessControl;
+import org.apache.storm.generated.AuthorizationException;
+import org.apache.storm.generated.KeyAlreadyExistsException;
+import org.apache.storm.generated.KeyNotFoundException;
+import org.apache.storm.generated.SettableBlobMeta;
+import org.apache.storm.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+
+public class DependencyUploader {
+ public static final Logger LOG = LoggerFactory.getLogger(DependencyUploader.class);
+
+ private final Map<String, Object> conf;
+ private ClientBlobStore blobStore;
+
+ public DependencyUploader() {
+ conf = Utils.readStormConfig();
+ }
+
+ public void init() {
+ if (blobStore == null) {
+ blobStore = Utils.getClientBlobStore(conf);
+ }
+ }
+
+ public void shutdown() {
+ if (blobStore != null) {
+ blobStore.shutdown();
+ }
+ }
+
+ @VisibleForTesting
+ void setBlobStore(ClientBlobStore blobStore) {
+ this.blobStore = blobStore;
+ }
+
+ @SuppressWarnings("unchecked")
+ public List<String> uploadFiles(List<File> dependencies, boolean cleanupIfFails) throws IOException, AuthorizationException {
+ checkFilesExist(dependencies);
+
+ List<String> keys = new ArrayList<>(dependencies.size());
+ try {
+ for (File dependency : dependencies) {
+ String fileName = dependency.getName();
+ String key = BlobStoreUtils.generateDependencyBlobKey(BlobStoreUtils.applyUUIDToFileName(fileName));
+
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // it should never happened since we apply UUID
+ throw new RuntimeException(e);
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ if (blobStore != null && cleanupIfFails) {
+ deleteBlobs(keys);
+ }
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ public List<String> uploadArtifacts(Map<String, File> artifacts) {
+ checkFilesExist(artifacts.values());
+
+ List<String> keys = new ArrayList<>(artifacts.size());
+ try {
+ for (Map.Entry<String, File> artifactToFile : artifacts.entrySet()) {
+ String artifact = artifactToFile.getKey();
+ File dependency = artifactToFile.getValue();
+
+ String key = BlobStoreUtils.generateDependencyBlobKey(convertArtifactToJarFileName(artifact));
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // we lose the race, but it doesn't matter
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ private String convertArtifactToJarFileName(String artifact) {
+ return artifact.replace(":", "-") + ".jar";
+ }
+
+ private boolean uploadDependencyToBlobStore(String key, File dependency)
+ throws KeyAlreadyExistsException, AuthorizationException, IOException {
+
+ boolean uploadNew = false;
+ try {
+ // FIXME: we can filter by listKeys() with local blobstore when STORM-1986 is going to be resolved
+ // as a workaround, we call getBlobMeta() for all keys
+ blobStore.getBlobMeta(key);
+ } catch (KeyNotFoundException e) {
+ // TODO: do we want to add ACL here?
--- End diff --
Before that, we need to check that nimbus and supervisor can access these blobs. I didn't look deeply with blobstore so I would like someone to confirm this.
@revans2 Could you guide this thing?
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1608
+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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd
Yes I've excluded that for simplification. Since this is a new feature I'd love to show that this concept works.
Moreover, in order to do that Nimbus should be modified but I also concerned with Nimbus port work. If we (including @longdafeng) are OK to address this before porting I'll do that work in this PR.
Btw, since jars are not shared but packages and transitive dependencies are meant to be shared, we should handle them different. We should remove blobs which are passed to --jars.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73984302
--- Diff: storm-core/src/clj/org/apache/storm/daemon/supervisor.clj ---
@@ -893,29 +891,61 @@
(let [rsrc-file-path (File. (.getFilePath local-rsrc))
key-name (.getName rsrc-file-path)
blob-symlink-target-name (.getName (File. (.getCurrentSymlinkPath local-rsrc)))
- symlink-name (get-blob-localname (get blobstore-map key-name) key-name)]
+ symlink-name (fn-symlink-name blobs key-name)]
(create-symlink! tmproot (.getParent rsrc-file-path) symlink-name
- blob-symlink-target-name))))
+ blob-symlink-target-name))))
(catch AuthorizationException authExp
(log-error authExp))
(catch KeyNotFoundException knf
(log-error knf))))))
+(defn download-blobs-in-blobstore-map-for-topology!
+ "Download all blobs listed in the topology configuration for a given topology."
+ [conf stormconf-path localizer tmproot]
+ (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
+ blobstore-map (storm-conf TOPOLOGY-BLOBSTORE-MAP)]
+ (download-blobs-for-topology! conf storm-conf localizer tmproot blobstore-map
+ (fn [blobs] (blobstore-map-to-localresources blobs))
+ (fn [blobs key-name] (get-blob-localname (get blobs key-name) key-name)))))
+
+(defn download-dependencies-for-topology!
+ "Download all dependencies blobs listed in the topology configuration for a given topology."
+ [conf stormconf-path stormcode-path localizer tmproot]
+ (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
+ storm-code (read-supervisor-storm-code-given-path stormcode-path)
+ dependencies (concat (.get_dependency_jars ^StormTopology storm-code)
+ (.get_dependency_artifacts ^StormTopology storm-code))]
+ (download-blobs-for-topology! conf storm-conf localizer tmproot dependencies
+ (fn [blobs] (map #(LocalResource. % false) blobs))
+ (fn [_ key-name] key-name))))
+
(defn get-blob-file-names
[blobstore-map]
(if blobstore-map
(for [[k, data] blobstore-map]
(get-blob-localname data k))))
(defn download-blobs-for-topology-succeed?
+ [target-dir file-names]
+ (if-not (empty? file-names)
+ (every? #(Utils/checkFileExists target-dir %) file-names)
+ true))
+
+(defn download-blobs-in-blobstore-map-for-topology-succeed?
--- End diff --
I don't mind if it's private or not, since we already ported this to Java in master branch.
There're other functions which could be private.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74009279
--- Diff: storm-core/src/jvm/org/apache/storm/dependency/DependencyUploader.java ---
@@ -0,0 +1,166 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.dependency;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.commons.lang.StringUtils;
+import org.apache.storm.blobstore.AtomicOutputStream;
+import org.apache.storm.blobstore.BlobStoreUtils;
+import org.apache.storm.blobstore.ClientBlobStore;
+import org.apache.storm.generated.AccessControl;
+import org.apache.storm.generated.AuthorizationException;
+import org.apache.storm.generated.KeyAlreadyExistsException;
+import org.apache.storm.generated.KeyNotFoundException;
+import org.apache.storm.generated.SettableBlobMeta;
+import org.apache.storm.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+
+public class DependencyUploader {
+ public static final Logger LOG = LoggerFactory.getLogger(DependencyUploader.class);
+
+ private final Map<String, Object> conf;
+ private ClientBlobStore blobStore;
+
+ public DependencyUploader() {
+ conf = Utils.readStormConfig();
+ }
+
+ public void init() {
+ if (blobStore == null) {
+ blobStore = Utils.getClientBlobStore(conf);
+ }
+ }
+
+ public void shutdown() {
+ if (blobStore != null) {
+ blobStore.shutdown();
+ }
+ }
+
+ @VisibleForTesting
+ void setBlobStore(ClientBlobStore blobStore) {
+ this.blobStore = blobStore;
+ }
+
+ @SuppressWarnings("unchecked")
+ public List<String> uploadFiles(List<File> dependencies, boolean cleanupIfFails) throws IOException, AuthorizationException {
+ checkFilesExist(dependencies);
+
+ List<String> keys = new ArrayList<>(dependencies.size());
+ try {
+ for (File dependency : dependencies) {
+ String fileName = dependency.getName();
+ String key = BlobStoreUtils.generateDependencyBlobKey(BlobStoreUtils.applyUUIDToFileName(fileName));
+
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // it should never happened since we apply UUID
+ throw new RuntimeException(e);
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ if (blobStore != null && cleanupIfFails) {
+ deleteBlobs(keys);
+ }
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ public List<String> uploadArtifacts(Map<String, File> artifacts) {
+ checkFilesExist(artifacts.values());
+
+ List<String> keys = new ArrayList<>(artifacts.size());
+ try {
+ for (Map.Entry<String, File> artifactToFile : artifacts.entrySet()) {
+ String artifact = artifactToFile.getKey();
+ File dependency = artifactToFile.getValue();
+
+ String key = BlobStoreUtils.generateDependencyBlobKey(convertArtifactToJarFileName(artifact));
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // we lose the race, but it doesn't matter
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ private String convertArtifactToJarFileName(String artifact) {
+ return artifact.replace(":", "-") + ".jar";
+ }
+
+ private boolean uploadDependencyToBlobStore(String key, File dependency)
+ throws KeyAlreadyExistsException, AuthorizationException, IOException {
+
+ boolean uploadNew = false;
+ try {
+ // FIXME: we can filter by listKeys() with local blobstore when STORM-1986 is going to be resolved
+ // as a workaround, we call getBlobMeta() for all keys
+ blobStore.getBlobMeta(key);
+ } catch (KeyNotFoundException e) {
+ // TODO: do we want to add ACL here?
--- End diff --
I do not think we should have acls for artifacts. If it is possible to support acls in this scenario, it should be done only for jars option.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74005665
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
+ Collection<Exclusion> exclusions = new ArrayList<>();
+ for (int idx = 1 ; idx < dependencyAndExclusions.size() ; idx++) {
+ exclusions.add(AetherUtils.createExclusion(dependencyAndExclusions.get(idx)));
+ }
+
+ Artifact artifact = new DefaultArtifact(dependencyAndExclusions.get(0));
+ return new Dependency(artifact, JavaScopes.COMPILE, false, exclusions);
+ }
+
+ public static Exclusion createExclusion(String exclusionString) {
+ String[] parts = exclusionString.split(":");
+
+ // length of parts should be greater than 0
+ String groupId = parts[0];
+
+ String artifactId = "*";
+ String classifier = "*";
+ String extension = "*";
+
+ int len = parts.length;
+ if (len > 1) {
+ artifactId = parts[1];
+ }
+ if (len > 2) {
+ classifier = parts[2];
+ }
+ if (len > 3) {
+ extension = parts[3];
+ }
+
+ return new Exclusion(groupId, artifactId, classifier, extension);
+ }
+
+ public static String artifactToString(Artifact artifact) {
+ StringBuilder buffer = new StringBuilder(128);
+ buffer.append(artifact.getGroupId());
+ buffer.append(':').append(artifact.getArtifactId());
+ buffer.append(':').append(artifact.getExtension());
+ if(artifact.getClassifier().length() > 0) {
--- End diff --
OK will address.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
I found a bug while testing other stuff with applied cluster.
StormSubmitter doesn't handle empty --jars option properly. Will address this shortly both this and PR for master branch.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 This issue was raised in the proposal discussion [here](https://mail-archives.apache.org/mod_mbox/storm-dev/201608.mbox/%3CCAF5108hbsHL3MO6HU47ncayV%2BgcMZ1s-B2izvRFmL7ot-TcqJg%40mail.gmail.com%3E). I do not think that proposal handles removing the blobs.
@HeartSaVioR Do you want to address this issue in this PR or you have plans to address it later?
I have not yet started reviewing this PR, I will try to review it soon.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73821960
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
+ jars_list.extend(curr.pop().split(','))
+ elif token == "--artifacts":
--- End diff --
Is "artifacts" a maven specific word ? How about "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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73706186
--- Diff: bin/storm.py ---
@@ -154,6 +157,39 @@ def confvalue(name, extrapaths, daemon=True):
return " ".join(tokens[1:])
return ""
+def resolve_dependencies(packages):
--- End diff --
Addressed.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Merged via df8346ce1a61ccad2a0c3f715b6c3a53bdaf5670
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang Thanks for checking the details of documentation.
Please leave a comment when you're done reviewing of this. Thanks!
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
[STORM-2016](http://issues.apache.org/jira/browse/STORM-2016) can also resolve [STORM-1435](http://issues.apache.org/jira/browse/STORM-1435) via applying different strategy, specifying dependencies to --jars and --packages instead of creating uber jar.
(Yes in fact STORM-2016 is started to resolve STORM-1435.)
Note that `storm sql` adds storm-sql-core and its transitive dependencies to classpath to run Runner, and also adds storm-sql-runtime and its transitive dependencies to classpath to run topology by adding them to --jars option.
With [STORM-2023](https://issues.apache.org/jira/browse/STORM-2023) (#1610), we can run the example of storm-sql by below command.
> ./bin/storm sql order_filtering.sql order_filtering --packages "org.apache.storm:storm-sql-kafka:1.1.1-SNAPSHOT,org.apache.storm:storm-kafka:1.1.1-SNAPSHOT,org.apache.kafka:kafka_2.10:0.8.2.2\!org.slf4j:slf4j-log4j12,org.apache.kafka:kafka-clients:0.8.2.2"
Without STORM-2023, we still have a chance to run the example of storm-sql by below command
> ./bin/storm sql order_filtering.sql order_filtering --packages "org.apache.storm:storm-sql-kafka:1.1.1-SNAPSHOT,org.apache.storm:storm-kafka:1.1.1-SNAPSHOT,org.apache.kafka:kafka_2.10:0.8.2.2\!org.slf4j:slf4j-log4j12,org.apache.kafka:kafka-clients:0.8.2.2,org.apache.calcite:calcite-core:1.4.0-incubating\!commons-dbcp:commons-dbcp\!com.google.code.findbugs:jsr305\!org.codehaus.janino:janino\!org.codehaus.janino:commons-compiler\!org.pentaho:pentaho-aggdesigner-algorithm\!com.fasterxml.jackson.core:jackson-annotations"
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74366031
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
+ jars_list.extend(curr.pop().split(','))
+ elif token == "--artifacts":
--- End diff --
whats the intention behind artifacts?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73824049
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
And yes we can have example on this. Will address.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73823437
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
neither system provided nor included to application 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 pull request #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74033001
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
--- End diff --
Addressed.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74000389
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/command/DependencyResolverMain.java ---
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.submit.command;
+
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.apache.storm.submit.dependency.AetherUtils;
+import org.apache.storm.submit.dependency.DependencyResolver;
+import org.json.simple.JSONValue;
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.resolution.ArtifactResult;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+public class DependencyResolverMain {
+
+ public static void main(String[] args) {
+ if (args.length < 1) {
+ throw new IllegalArgumentException("packages must be presented.");
--- End diff --
artifacts now ? Same for error message below
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR - not sure if it will be of help, but you can also checkout https://github.com/apache/storm/pull/1296/files. I was trying out something similar.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@harshach @satishd @manuzhang
I just fixed one thing from nimbus.clj: just log exception instead of crashing nimbus when `blob-rm-dependency-jars-in-topology` throws Exception. Before that nimbus test is failing intermittent.
You can just review nimbus.clj once again. 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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@satishd
Thanks for the thoughtful review and vote. I rebased the commits into one.
@revans2 @manuzhang @abhishekagarwal87
Please continue reviewing. I'll merge this by tomorrow if there's no more discussion. Thanks.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74004298
--- Diff: storm-core/src/jvm/org/apache/storm/dependency/DependencyUploader.java ---
@@ -0,0 +1,166 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.dependency;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.commons.lang.StringUtils;
+import org.apache.storm.blobstore.AtomicOutputStream;
+import org.apache.storm.blobstore.BlobStoreUtils;
+import org.apache.storm.blobstore.ClientBlobStore;
+import org.apache.storm.generated.AccessControl;
+import org.apache.storm.generated.AuthorizationException;
+import org.apache.storm.generated.KeyAlreadyExistsException;
+import org.apache.storm.generated.KeyNotFoundException;
+import org.apache.storm.generated.SettableBlobMeta;
+import org.apache.storm.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+
+public class DependencyUploader {
+ public static final Logger LOG = LoggerFactory.getLogger(DependencyUploader.class);
+
+ private final Map<String, Object> conf;
+ private ClientBlobStore blobStore;
+
+ public DependencyUploader() {
+ conf = Utils.readStormConfig();
+ }
+
+ public void init() {
+ if (blobStore == null) {
+ blobStore = Utils.getClientBlobStore(conf);
+ }
+ }
+
+ public void shutdown() {
+ if (blobStore != null) {
+ blobStore.shutdown();
+ }
+ }
+
+ @VisibleForTesting
+ void setBlobStore(ClientBlobStore blobStore) {
+ this.blobStore = blobStore;
+ }
+
+ @SuppressWarnings("unchecked")
+ public List<String> uploadFiles(List<File> dependencies, boolean cleanupIfFails) throws IOException, AuthorizationException {
+ checkFilesExist(dependencies);
+
+ List<String> keys = new ArrayList<>(dependencies.size());
+ try {
+ for (File dependency : dependencies) {
+ String fileName = dependency.getName();
+ String key = BlobStoreUtils.generateDependencyBlobKey(BlobStoreUtils.applyUUIDToFileName(fileName));
+
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // it should never happened since we apply UUID
+ throw new RuntimeException(e);
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ if (blobStore != null && cleanupIfFails) {
+ deleteBlobs(keys);
+ }
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ public List<String> uploadArtifacts(Map<String, File> artifacts) {
+ checkFilesExist(artifacts.values());
+
+ List<String> keys = new ArrayList<>(artifacts.size());
+ try {
+ for (Map.Entry<String, File> artifactToFile : artifacts.entrySet()) {
+ String artifact = artifactToFile.getKey();
+ File dependency = artifactToFile.getValue();
+
+ String key = BlobStoreUtils.generateDependencyBlobKey(convertArtifactToJarFileName(artifact));
+ try {
+ uploadDependencyToBlobStore(key, dependency);
+ } catch (KeyAlreadyExistsException e) {
+ // we lose the race, but it doesn't matter
+ }
+
+ keys.add(key);
+ }
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+
+ return keys;
+ }
+
+ private String convertArtifactToJarFileName(String artifact) {
+ return artifact.replace(":", "-") + ".jar";
+ }
+
+ private boolean uploadDependencyToBlobStore(String key, File dependency)
+ throws KeyAlreadyExistsException, AuthorizationException, IOException {
+
+ boolean uploadNew = false;
+ try {
+ // FIXME: we can filter by listKeys() with local blobstore when STORM-1986 is going to be resolved
+ // as a workaround, we call getBlobMeta() for all keys
+ blobStore.getBlobMeta(key);
+ } catch (KeyNotFoundException e) {
+ // TODO: do we want to add ACL here?
--- End diff --
Can we file JIRA for this? This is a low priority to have any acls user may want to provide.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73825749
--- Diff: docs/Command-line-client.md ---
@@ -43,6 +43,19 @@ Syntax: `storm jar topology-jar-path class ...`
Runs the main method of `class` with the specified arguments. The storm jars and configs in `~/.storm` are put on the classpath. The process is configured so that [StormSubmitter](javadocs/org/apache/storm/StormSubmitter.html) will upload the jar at `topology-jar-path` when the topology is submitted.
+When you want to ship other jars which is not included to application jar, you can pass them to `--jars` option with comma-separated string.
--- End diff --
It would be ideal if we can show same artifacts with different versions as warning, but I'm not sure it would be possible, especially application jar is an uber jar so we can't get artifacts within that jar.
If you mean we just would be better to show some general warnings, yes that would be good to have. Btw, this is closer to general behavior of determining worker classpath (even without these options), so it would be better to explain this in general manner.
I'm not sure where is the best place to address this. Let me think about it.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73984542
--- Diff: storm-core/src/clj/org/apache/storm/daemon/supervisor.clj ---
@@ -875,14 +875,12 @@
(worker-launcher-and-wait conf (storm-conf TOPOLOGY-SUBMITTER-USER) ["blob" path] :log-prefix (str "setup blob permissions for " path))))
(defn download-blobs-for-topology!
- "Download all blobs listed in the topology configuration for a given topology."
- [conf stormconf-path localizer tmproot]
- (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
- blobstore-map (storm-conf TOPOLOGY-BLOBSTORE-MAP)
+ [conf storm-conf localizer tmproot blobs fn-local-resources fn-symlink-name]
--- End diff --
OK I'll do.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Please hold off merging. I'll create pull request against master. We can merge both of them together.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74032928
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -238,39 +239,24 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
// Dependency uploading only makes sense for distributed mode
- List<String> jarsBlobKeys;
+ List<String> jarsBlobKeys = Collections.emptyList();
List<String> artifactsBlobKeys;
DependencyUploader uploader = new DependencyUploader();
try {
uploader.init();
jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
+ setDependencyBlobsToTopology(topology, jarsBlobKeys, artifactsBlobKeys);
+ submitTopologyInDistributeMode(name, topology, opts, progressListener, asUser, conf, serConf);
+ } catch (Throwable e) {
+ // remove uploaded jars blobs, not artifacts since they're shared across the cluster
+ uploader.deleteBlobs(jarsBlobKeys);
--- End diff --
Addressed.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR Overall LGTM. I will revisit once the comments are addressed/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.
---
[GitHub] storm pull request #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR closed the pull request at:
https://github.com/apache/storm/pull/1608
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73841656
--- Diff: storm-core/src/clj/org/apache/storm/daemon/supervisor.clj ---
@@ -875,14 +875,12 @@
(worker-launcher-and-wait conf (storm-conf TOPOLOGY-SUBMITTER-USER) ["blob" path] :log-prefix (str "setup blob permissions for " path))))
(defn download-blobs-for-topology!
- "Download all blobs listed in the topology configuration for a given topology."
- [conf stormconf-path localizer tmproot]
- (let [storm-conf (read-supervisor-storm-conf-given-path conf stormconf-path)
- blobstore-map (storm-conf TOPOLOGY-BLOBSTORE-MAP)
+ [conf storm-conf localizer tmproot blobs fn-local-resources fn-symlink-name]
--- End diff --
update the comments instead of removing it ?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73840557
--- Diff: storm-core/src/jvm/org/apache/storm/generated/Assignment.java ---
@@ -787,15 +787,15 @@ public void read(org.apache.thrift.protocol.TProtocol iprot, Assignment struct)
case 2: // NODE_HOST
if (schemeField.type == org.apache.thrift.protocol.TType.MAP) {
{
- org.apache.thrift.protocol.TMap _map548 = iprot.readMapBegin();
- struct.node_host = new HashMap<String,String>(2*_map548.size);
- String _key549;
- String _val550;
- for (int _i551 = 0; _i551 < _map548.size; ++_i551)
+ org.apache.thrift.protocol.TMap _map564 = iprot.readMapBegin();
--- End diff --
Forgive me for a dumb and irrelevant question. Why do we keep generated files in source ?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74028593
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
--- End diff --
I think --jars is representing its meaning properly so I'd like to keep it as it is.
If there's other one which would want to change option to non-duplicated name, I'll do that.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73826382
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
--- End diff --
It's a bit strange to have `storm jar --jars`. Maybe `--dep-jars` ?
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR yes, I did so and verified I have all the dependencies locally. I also modified the `DependencyResolverTest` to resolve `1.1.1-SNAPSHOT` and the UT passed.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74000507
--- Diff: external/storm-submit/src/main/java/org/apache/storm/submit/dependency/AetherUtils.java ---
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.submit.dependency;
+
+import org.sonatype.aether.artifact.Artifact;
+import org.sonatype.aether.graph.Dependency;
+import org.sonatype.aether.graph.Exclusion;
+import org.sonatype.aether.util.artifact.DefaultArtifact;
+import org.sonatype.aether.util.artifact.JavaScopes;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+public class AetherUtils {
+ private AetherUtils() {
+ }
+
+ public static Dependency parseDependency(String dependency) {
+ List<String> dependencyAndExclusions = Arrays.asList(dependency.split("!"));
--- End diff --
"!" is a special bach character that refers to previous command. Although we can escape it with "\", is there a better choice ?
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
OK, I created pull request against master: #1626
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74031650
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -238,39 +239,24 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
// Dependency uploading only makes sense for distributed mode
- List<String> jarsBlobKeys;
+ List<String> jarsBlobKeys = Collections.emptyList();
List<String> artifactsBlobKeys;
DependencyUploader uploader = new DependencyUploader();
try {
uploader.init();
jarsBlobKeys = uploadDependencyJarsToBlobStore(uploader);
artifactsBlobKeys = uploadDependencyArtifactsToBlobStore(uploader);
+ setDependencyBlobsToTopology(topology, jarsBlobKeys, artifactsBlobKeys);
+ submitTopologyInDistributeMode(name, topology, opts, progressListener, asUser, conf, serConf);
+ } catch (Throwable e) {
+ // remove uploaded jars blobs, not artifacts since they're shared across the cluster
+ uploader.deleteBlobs(jarsBlobKeys);
--- End diff --
@satishd
Yes I think the scenarios you stated can be occurred. But also Nimbus throws TException when Nimbus is unable to handle the submission, say, runtime exception occurred.
So it's more like loosely catching with less risk vs hardly catching with more risk.
(Risk could be reduced via checking topology name once again indeed.)
After thinking it again, leaving blobs seems less harmful than whole contents of topology couldn't be downloaded at all. I'll follow your suggestion.
---
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 #1608: [WIP] STORM-2016 Topology submission improvement: support...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Changed to \[WIP] since there're failures of unit tests. I'll take care of that so please continue reviewing pull request.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73822027
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
--- End diff --
Are these two options common for commands ? If so, could we have some help info in the `print_command` function ?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r74367408
--- Diff: bin/storm.py ---
@@ -232,44 +271,91 @@ def jar(jarfile, klass, *args):
The process is configured so that StormSubmitter
(http://storm.apache.org/releases/current/javadocs/org/apache/storm/StormSubmitter.html)
will upload the jar at topology-jar-path when the topology is submitted.
+
+ When you want to ship other jars which is not included to application jar, you can pass them to --jars option with comma-separated string.
+ For example, --jars "your-local-jar.jar,your-local-jar2.jar" will load your-local-jar.jar and your-local-jar2.jar.
+ And when you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
+ You can also exclude some dependencies like what you're doing in maven pom.
+ Please add exclusion artifacts with '^' separated string after the artifact.
+ For example, --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12" will load jedis and kafka artifact and all of transitive dependencies but exclude slf4j-log4j12 from kafka.
+
+ Complete example of both options is here: `./bin/storm jar example/storm-starter/storm-starter-topologies-*.jar org.apache.storm.starter.RollingTopWords blobstore-remote2 remote --jars "./external/storm-redis/storm-redis-1.1.0.jar,./external/storm-kafka/storm-kafka-1.1.0.jar" --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka_2.10:0.8.2.2^org.slf4j:slf4j-log4j12"`
+
+ When you pass jars and/or artifacts options, StormSubmitter will upload them when the topology is submitted, and they will be included to classpath of both the process which runs the class, and also workers for that topology.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
transform_class = confvalue("client.jartransformer.class", [CLUSTER_CONF_DIR])
if (transform_class != None and transform_class != "nil"):
tmpjar = os.path.join(tempfile.gettempdir(), uuid.uuid1().hex+".jar")
exec_storm_class("org.apache.storm.daemon.ClientJarTransformerRunner", args=[transform_class, jarfile, tmpjar], fork=True, daemon=False)
+ extra_jars = [tmpjar, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
topology_runner_exit_code = exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
fork=True,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + tmpjar] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
os.remove(tmpjar)
sys.exit(topology_runner_exit_code)
else:
+ extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+ extra_jars.extend(local_jars)
+ extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
klass,
jvmtype="-client",
- extrajars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR],
+ extrajars=extra_jars,
args=args,
daemon=False,
- jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile])
+ jvmopts=JAR_JVM_OPTS + ["-Dstorm.jar=" + jarfile] +
+ ["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
+ ["-Dstorm.dependency.artifacts=" + json.dumps(artifact_to_file_jars)])
def sql(sql_file, topology_name):
"""Syntax: [storm sql sql-file topology-name]
Compiles the SQL statements into a Trident topology and submits it to Storm.
+
+ --jars and --artifacts options available for jar are also applied to sql command.
+ Please refer "help jar" to see how to use --jars and --artifacts options.
+ You normally want to pass these options since you need to set data source to your sql which is an external storage in many cases.
"""
+ global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS
+
+ local_jars = DEP_JARS_OPTS
+ artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS)
+
+ sql_core_jars = get_jars_full(STORM_DIR + "/external/sql/storm-sql-core")
--- End diff --
Definitely. I've also stated that thing to somewhere other pull requests.
`external` has various kinds of modules, and we need to arrange that.
Maybe we can leave kind of connectors as it is (we can even rename on this), and get flux, sql, kafka-monitor (this module is referred from script in bin, too), and more to out of external.
Since this issue is not only regarding to storm-sql I would like to initiate the discussion before making the change, and handle this to another issue. What do you think?
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73822799
--- Diff: bin/storm.py ---
@@ -754,18 +827,24 @@ def parse_config_opts(args):
elif token == "--config":
global CONFFILE
CONFFILE = curr.pop()
+ elif token == "--jars":
--- End diff --
It's only used for `storm jar` and `storm sql`, but showing them to `print_command` would be better. I'll address this.
Thanks for the suggestion.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Rebased with current 1.x-branch.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by manuzhang <gi...@git.apache.org>.
Github user manuzhang commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73985164
--- Diff: storm-core/src/clj/org/apache/storm/config.clj ---
@@ -141,6 +141,10 @@
[conf stormconf-path]
(merge conf (clojurify-structure (Utils/fromCompressedJsonConf (FileUtils/readFileToByteArray (File. stormconf-path))))))
+(defn read-supervisor-storm-code-given-path
--- End diff --
maybe update the `read-supervisor-topology` function below to use this
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1608
Conceptually I like the idea, and overall the code looks good. My biggest concern is around cleanup of the bobs after the topology finishes. I don't see anywhere in the code that it is doing that, but I didn't dig too deeply into it.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd
Addressed removing jars when topology is killed. Please note that artifacts will be not removed since it's shared among whole topologies.
Since it modifies Nimbus, we can still merge this in without this feature, but I don't want to be blocked by the thing even pull request is not submitted.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Rebased with current 1.x-branch again.
---
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 #1608: STORM-2016 Topology submission improvement: suppor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1608#discussion_r73842378
--- Diff: storm-core/src/jvm/org/apache/storm/generated/Assignment.java ---
@@ -787,15 +787,15 @@ public void read(org.apache.thrift.protocol.TProtocol iprot, Assignment struct)
case 2: // NODE_HOST
if (schemeField.type == org.apache.thrift.protocol.TType.MAP) {
{
- org.apache.thrift.protocol.TMap _map548 = iprot.readMapBegin();
- struct.node_host = new HashMap<String,String>(2*_map548.size);
- String _key549;
- String _val550;
- for (int _i551 = 0; _i551 < _map548.size; ++_i551)
+ org.apache.thrift.protocol.TMap _map564 = iprot.readMapBegin();
--- End diff --
There's no dumb question. This is just a one of practices on Storm project. As always there're some pros and cons regarding this:
pros: Thrift compiler is only needed for contributors who modifies storm.thrift, and the case is actually not often.
cons: Thrift generates codes which makes huge diff even though just adding only one field.
IMO, installing thrift compiler because of just build project is bad, especially OSPs are using different versions of thrift. While disadvantage is also not a trivial, I think advantage is valuable enough.
---
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 #1608: STORM-2016 Topology submission improvement: support addin...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Added documentation, and replace packages to artifacts since it's clearer for its meaning.
I think it's done with 1.x branch, so I'll go on porting this to master branch.
---
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.
---