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/12 14:05:16 UTC
[GitHub] storm pull request #1626: STORM-2016 Topology submission improvement: suppor...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1626
STORM-2016 Topology submission improvement: support adding local jars and maven artifacts on submission
PR for 1.x is here: #1608
storm-submit is copied version of 1.x, but I have to reimplement (port to Java) some part of storm-core. So please have a look thoughtfully when reviewing files on storm-core.
I also fixed a bug from outside of pull request regarding symbolic link.
@harshach @satishd
Please review this when you have some time. Thanks!
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2016
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1626.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 #1626
----
commit 97615a4b5e9c3567ffd5b0642b60fc8e55e713ef
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2016-08-12T13:58:08Z
STORM-2016 Topology submission improvement: support adding local jars and maven artifacts on submission
*NOTE* This contains a bugfix at creating symbolic link
* bin/storm now supports "--jars" and "--artifacts" 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
** StormSubmitter will remove jars blobs when topology submission fails
*** remove jar blobs when only one of AlreadyAliveException, InvalidTopologyException, AuthorizationException occurs
* Supervisor will download dependencies from BlobStore when such topology is assigned
* Supervisor will launch workers with adding downloaded dependencies to worker classpath
* Nimbus will remove jars from BlobStore when topology is killed
** don't remove artifacts from blobstore since it's shared across topologies
* add options to document (Command-line-client.md) and also storm.py
** it will be printed out when 'help jar' and 'help sql' is called
----
---
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 #1626: 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/1626#discussion_r75471141
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -258,6 +276,65 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
+ private static List<String> uploadDependencyJarsToBlobStore(DependencyUploader uploader) {
+ LOG.info("Uploading dependencies - jars...");
+
+ DependencyPropertiesParser propertiesParser = new DependencyPropertiesParser();
+
+ String depJarsProp = System.getProperty("storm.dependency.jars", "");
+ List<File> depJars = propertiesParser.parseJarsProperties(depJarsProp);
+
+ try {
+ return uploader.uploadFiles(depJars, true);
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static List<String> uploadDependencyArtifactsToBlobStore(DependencyUploader uploader) {
+ LOG.info("Uploading dependencies - artifacts...");
+
+ DependencyPropertiesParser propertiesParser = new DependencyPropertiesParser();
+
+ String depArtifactsProp = System.getProperty("storm.dependency.artifacts", "{}");
+ Map<String, File> depArtifacts = propertiesParser.parseArtifactsProperties(depArtifactsProp);
+
+ try {
+ return uploader.uploadArtifacts(depArtifacts);
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static void setDependencyBlobsToTopology(StormTopology topology, List<String> jarsBlobKeys, List<String> artifactsBlobKeys) {
+ LOG.info("Dependency Blob keys - jars : {} / artifacts : {}", jarsBlobKeys, artifactsBlobKeys);
+ topology.set_dependency_jars(jarsBlobKeys);
+ topology.set_dependency_artifacts(artifactsBlobKeys);
+ }
+
+ private static void submitTopologyInDistributeMode(String name, StormTopology topology, SubmitOptions opts,
+ ProgressListener progressListener, String asUser, Map conf,
+ String serConf) throws TException {
+ String jar = submitJarAs(conf, System.getProperty("storm.jar"), progressListener, asUser);
+ try {
+ LOG.info("Submitting topology " + name + " in distributed mode with conf " + serConf);
+ NimbusClient client = NimbusClient.getConfiguredClientAs(conf, asUser);
--- End diff --
Nice catch. 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 #1626: 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/1626
@satishd I addressed or commented your review. Please let me know if my proposal for the names would be OK for you, or you would like to propose another name or just want to change to 'check*'.
---
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 #1626: 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/1626
Found forward reference in storm.thrift from STORM-1994. I just addressed that from here since generated files might bother us.
Please check the change for `storm.thrift`.
---
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 #1626: 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/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 #1626: 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/1626#discussion_r75434061
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SyncSupervisorEvent.java ---
@@ -366,14 +386,29 @@ protected boolean didDownloadBlobsForTopologySucceed(String stormconfPath, Strin
blobFileNames.add(ret);
}
}
- for (String string : blobFileNames) {
- if (!Utils.checkFileExists(string))
- return false;
- }
- return true;
+
+ return didDownloadBlobsForTopologySucceed(targetDir, blobFileNames);
}
/**
+ * Assert if all dependencies blobs are downloaded for the given topology
+ *
+ * @param stormcodePath
+ * @param targetDir
+ * @return
+ */
+ protected boolean didDownloadDependenciesForTopologySucceed(String stormcodePath, String targetDir) throws IOException {
--- End diff --
Can we rename this method to more comprehensible name like checkForDownloadedDependencyFiles or any other sensible method name?
---
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 #1626: 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/1626#discussion_r75471082
--- Diff: external/storm-submit/pom.xml ---
@@ -0,0 +1,199 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>storm-submit</artifactId>
--- End diff --
I was actually named this as `storm-submitter` but it clashes with StormSubmitter class.
I named this when I planned to create something like spark-submit, but now it only handles dependency resolving (I mean it contains just a tool - or tools for later), so `storm-submit-tools` would be fine for me now.
---
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 #1626: 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/1626#discussion_r75471422
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SyncSupervisorEvent.java ---
@@ -366,14 +386,29 @@ protected boolean didDownloadBlobsForTopologySucceed(String stormconfPath, Strin
blobFileNames.add(ret);
}
}
- for (String string : blobFileNames) {
- if (!Utils.checkFileExists(string))
- return false;
- }
- return true;
+
+ return didDownloadBlobsForTopologySucceed(targetDir, blobFileNames);
}
/**
+ * Assert if all dependencies blobs are downloaded for the given topology
+ *
+ * @param stormcodePath
+ * @param targetDir
+ * @return
+ */
+ protected boolean didDownloadDependenciesForTopologySucceed(String stormcodePath, String targetDir) throws IOException {
--- End diff --
`areDependencyFilesDownloaded` would be fine for me since we're commenting like that.
Personally `check*` feels assertion which throws some exceptions if predicate doesn't hold true, but it's just me.
---
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 #1626: 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/1626#discussion_r75434179
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SyncSupervisorEvent.java ---
@@ -344,12 +347,29 @@ private void downloadDistributeStormCode(Map conf, String stormId, String master
/**
* Assert if all blobs are downloaded for the given topology
- *
+ *
+ * @param targetDir
+ * @param blobFileNames
+ * @return
+ */
+ protected boolean didDownloadBlobsForTopologySucceed(String targetDir, List<String> blobFileNames) throws IOException {
+ for (String string : blobFileNames) {
+ if (!Utils.checkFileExists(targetDir, string)) {
+ LOG.info("Fail to find downloaded file: dir {} filename {}", targetDir, string);
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Assert if all blobs in blobstore map are downloaded for the given topology
+ *
* @param stormconfPath
* @param targetDir
* @return
*/
- protected boolean didDownloadBlobsForTopologySucceed(String stormconfPath, String targetDir) throws IOException {
+ protected boolean didDownloadBlobsInBlobStoreMapForTopologySucceed(String stormconfPath, String targetDir) throws IOException {
--- End diff --
Can we rename this method to more comprehensible method name like it is suggested in other comment?
---
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 #1626: 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/1626#discussion_r75471479
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SyncSupervisorEvent.java ---
@@ -344,12 +347,29 @@ private void downloadDistributeStormCode(Map conf, String stormId, String master
/**
* Assert if all blobs are downloaded for the given topology
- *
+ *
+ * @param targetDir
+ * @param blobFileNames
+ * @return
+ */
+ protected boolean didDownloadBlobsForTopologySucceed(String targetDir, List<String> blobFileNames) throws IOException {
+ for (String string : blobFileNames) {
+ if (!Utils.checkFileExists(targetDir, string)) {
+ LOG.info("Fail to find downloaded file: dir {} filename {}", targetDir, string);
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Assert if all blobs in blobstore map are downloaded for the given topology
+ *
* @param stormconfPath
* @param targetDir
* @return
*/
- protected boolean didDownloadBlobsForTopologySucceed(String stormconfPath, String targetDir) throws IOException {
+ protected boolean didDownloadBlobsInBlobStoreMapForTopologySucceed(String stormconfPath, String targetDir) throws IOException {
--- End diff --
Same as above, `areBlobsDownloaded` would be fine for me since we're commenting like 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 issue #1626: 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/1626
I'll merge this now since we had fairly long review for pull request on 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 issue #1626: 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/1626
Addressed all fixes from 1.x. It's in sync.
---
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 #1626: 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/1626
+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 pull request #1626: 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/1626#discussion_r75433909
--- Diff: storm-core/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -258,6 +276,65 @@ public static void submitTopologyAs(String name, Map stormConf, StormTopology to
}
+ private static List<String> uploadDependencyJarsToBlobStore(DependencyUploader uploader) {
+ LOG.info("Uploading dependencies - jars...");
+
+ DependencyPropertiesParser propertiesParser = new DependencyPropertiesParser();
+
+ String depJarsProp = System.getProperty("storm.dependency.jars", "");
+ List<File> depJars = propertiesParser.parseJarsProperties(depJarsProp);
+
+ try {
+ return uploader.uploadFiles(depJars, true);
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static List<String> uploadDependencyArtifactsToBlobStore(DependencyUploader uploader) {
+ LOG.info("Uploading dependencies - artifacts...");
+
+ DependencyPropertiesParser propertiesParser = new DependencyPropertiesParser();
+
+ String depArtifactsProp = System.getProperty("storm.dependency.artifacts", "{}");
+ Map<String, File> depArtifacts = propertiesParser.parseArtifactsProperties(depArtifactsProp);
+
+ try {
+ return uploader.uploadArtifacts(depArtifacts);
+ } catch (Throwable e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static void setDependencyBlobsToTopology(StormTopology topology, List<String> jarsBlobKeys, List<String> artifactsBlobKeys) {
+ LOG.info("Dependency Blob keys - jars : {} / artifacts : {}", jarsBlobKeys, artifactsBlobKeys);
+ topology.set_dependency_jars(jarsBlobKeys);
+ topology.set_dependency_artifacts(artifactsBlobKeys);
+ }
+
+ private static void submitTopologyInDistributeMode(String name, StormTopology topology, SubmitOptions opts,
+ ProgressListener progressListener, String asUser, Map conf,
+ String serConf) throws TException {
+ String jar = submitJarAs(conf, System.getProperty("storm.jar"), progressListener, asUser);
+ try {
+ LOG.info("Submitting topology " + name + " in distributed mode with conf " + serConf);
+ NimbusClient client = NimbusClient.getConfiguredClientAs(conf, asUser);
--- End diff --
`NimbusClient` should be closed in finally block to release respective resources. It is an Autocloseable instance and it can be closed with try-resources block.
---
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 #1626: 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/1626#discussion_r75434327
--- Diff: external/storm-submit/pom.xml ---
@@ -0,0 +1,199 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>storm-submit</artifactId>
--- End diff --
Better to have this as `storm-submitter`, more suggestions are welcome. We may want to have respective changes in 1.x-branch also.
---
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 #1626: 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/1626
@HeartSaVioR I am fine with the proposed name changes.
---
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 #1626: 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/1626
Merged via 51f52fe8d67d7aed6adcaf1ce5bf47e6261b5204
---
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 #1626: 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/1626
Rebased with latest master.
---
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 #1626: 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/1626
@satishd Addressed naming comments. I also modified 1.x PR for changing module name from storm-submit to storm-submit-tools.
---
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.
---