You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by knusbaum <gi...@git.apache.org> on 2016/06/21 15:45:51 UTC
[GitHub] storm pull request #1507: STORM-1916: Add ability for worker-first classpath
GitHub user knusbaum opened a pull request:
https://github.com/apache/storm/pull/1507
STORM-1916: Add ability for worker-first classpath
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/knusbaum/incubator-storm STORM-1916
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1507.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 #1507
----
commit 50444bc15da839f0f88d94bcbd160601a6bdb373
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T17:59:06Z
Working on cherry-pick
commit e197b0183c6a364b09f4d1b4c13694a4a6af1246
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-14T18:53:26Z
Almost there.
commit 526178924e722bc9390c9453225a7811f1aa6e2e
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:10:04Z
Doing pick.
commit 05b4aff8951265e5cfc3f58a7431db1a702aeddc
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:11:02Z
Removing intermediate files.
commit 82e67600fc0cfb9c2110816508f4971e2e3f15ab
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-17T18:26:16Z
Fixing stuff.
commit 193ab173c88e39b2167c688b4a613a4c14154bf1
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-06-21T15:43:47Z
Adding user-first 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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1507
New test looks good.
Build failures (one netty timeout, one windowed executor test failure) look unrelated to this change.
Looks like the only thing left to do is change the names of the new configs to be less confusing.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1507
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1507
@HeartSaVioR @abhishekagarwal87 Would you guys give this another look?
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1507
@HeartSaVioR @abhishekagarwal87
Yes, I accidentally included STORM-1913 in this PR. I'll remove 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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1507
Just some comments around renaming and adding some small 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 pull request #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69814156
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2092,6 +2092,11 @@ public static String addToClasspath(String classpath,
return _instance.addToClasspathImpl(classpath, paths);
}
+ public static String addToClasspath(Collection<String> classpaths,
+ Collection<String> paths) {
+ return _instance.addToClasspathImpl(classpaths, paths);
+ }
+
--- End diff --
Since we are making this static method mockable anyway, why don't we add a simple 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 pull request #1507: STORM-1916: Add ability for worker-first classpath
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69765983
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
--- End diff --
Can we rename it to something like `pre.topology.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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69813686
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
+
+ /**
+ * Enables user-first classpath. See topology.classpath.first
+ */
+ @isBoolean
+ public static final String STORM_USER_CLASSPATH_FIRST_ENABLED="storm.user.classpath.first.enabled";
--- End diff --
`storm.topology.classpath.beginning.enabled`?
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69772245
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2104,6 +2109,17 @@ public String addToClasspathImpl(String classpath,
return StringUtils.join(l, CLASS_PATH_SEPARATOR);
}
+ public String addToClasspathImpl(Collection<String> classpath,
+ Collection<String> paths) {
+ String basePath = StringUtils.join(classpath, CLASS_PATH_SEPARATOR);
--- End diff --
I think the implementation can be simpler.
```
List<String> allPaths = new ArrayList<>();
allPaths.addAll(classPaths)
allPaths.addAll(paths);
return StringUtils.join(allPaths, CLASS_PATH_SEPARATOR);
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69816548
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
--- End diff --
@d2r I like that 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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69813914
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/workermanager/DefaultWorkerManager.java ---
@@ -347,14 +347,25 @@ protected String getWorkerClassPath(String stormJar, Map stormConf) {
List<String> topoClasspath = new ArrayList<>();
Object object = stormConf.get(Config.TOPOLOGY_CLASSPATH);
+ // Will be populated only if STORM_USER_CLASSPATH_FIRST_ENABLED is set on Nimbus.
+ // Allowed for extreme debugging.
+ Object topologyClasspathFirst = stormConf.get(Config.TOPOLOGY_CLASSPATH_FIRST);
+ List<String> firstClasspathList = new ArrayList<>();
+ if(topologyClasspathFirst instanceof List) {
+ firstClasspathList.addAll((List<String>)topologyClasspathFirst);
--- End diff --
minor: indent to 4 spaces
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69764701
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2092,6 +2092,11 @@ public static String addToClasspath(String classpath,
return _instance.addToClasspathImpl(classpath, paths);
}
+ public static String addToClasspath(Collection<String> classpath,
--- End diff --
nit: rename `classPath` to 'classPaths'.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r70338370
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2092,6 +2092,11 @@ public static String addToClasspath(String classpath,
return _instance.addToClasspathImpl(classpath, paths);
}
+ public static String addToClasspath(Collection<String> classpaths,
+ Collection<String> paths) {
+ return _instance.addToClasspathImpl(classpaths, paths);
+ }
+
--- End diff --
Looks good.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69788000
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
--- End diff --
I don't think we want that one. Would `topology.classpath.prefix` or `topology.prefix.classpath` be better?
It should be `topology.*` because that's the 'namespace' that all topology configs live in.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69764774
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2104,6 +2109,17 @@ public String addToClasspathImpl(String classpath,
return StringUtils.join(l, CLASS_PATH_SEPARATOR);
}
+ public String addToClasspathImpl(Collection<String> classpath,
--- End diff --
nit: rename `classPath` to `classPaths`.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r70338269
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_BEGINNING="topology.classpath.beginning";
+
+ /**
+ * Enables user-first classpath. See topology.classpath.first
--- End diff --
comment rename
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69813543
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
--- End diff --
I agree with @satishd the name is strange. Maybe `topology.classpath.beginning`?
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69788392
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
+
+ /**
+ * Enables user-first classpath. See topology.classpath.first
+ */
+ @isBoolean
+ public static final String STORM_USER_CLASSPATH_FIRST_ENABLED="storm.user.classpath.first.enabled";
--- End diff --
This, unlike the above, should not be in the `topology.*` namespace, because `topology.*` configs are specific per-topology by convention.
Maybe `storm.topology.classpath.prefix.enabled`?
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1507
@knusbaum Could you make your branch not based on STORM-1913? I think we can handle them separately.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r70338254
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
--- End diff --
comment rename
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69977876
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2092,6 +2092,11 @@ public static String addToClasspath(String classpath,
return _instance.addToClasspathImpl(classpath, paths);
}
+ public static String addToClasspath(Collection<String> classpaths,
+ Collection<String> paths) {
+ return _instance.addToClasspathImpl(classpaths, paths);
+ }
+
--- End diff --
Done.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/1507
+1 after we fix the javadoc.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1507
Many changes in this PR look unrelated to STORM-1916. Does this PR combine other issues as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69797890
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/workermanager/DefaultWorkerManager.java ---
@@ -347,14 +347,24 @@ protected String getWorkerClassPath(String stormJar, Map stormConf) {
List<String> topoClasspath = new ArrayList<>();
Object object = stormConf.get(Config.TOPOLOGY_CLASSPATH);
+ // Will be populated only if STORM_USER_CLASSPATH_FIRST_ENABLED is set on Nimbus.
+ // Allowed for extreme debugging.
+ Object topologyClasspathFirst = stormConf.get(Config.TOPOLOGY_CLASSPATH_FIRST);
+ List<String> firstClasspathList = new ArrayList<>();
+ if(topologyClasspathFirst instanceof List) {
+ firstClasspathList.addAll((List<String>)topologyClasspathFirst);
+ } else if (topologyClasspathFirst instanceof String) {
+ firstClasspathList.add((String) topologyClasspathFirst);
+ }
--- End diff --
Done.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1507
Let's make sure that we squash the commits before merging.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1507
@d2r Sorry about missing that. Ready for a 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 pull request #1507: STORM-1916: Add ability for worker-first classpath
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69771410
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/workermanager/DefaultWorkerManager.java ---
@@ -347,14 +347,24 @@ protected String getWorkerClassPath(String stormJar, Map stormConf) {
List<String> topoClasspath = new ArrayList<>();
Object object = stormConf.get(Config.TOPOLOGY_CLASSPATH);
+ // Will be populated only if STORM_USER_CLASSPATH_FIRST_ENABLED is set on Nimbus.
+ // Allowed for extreme debugging.
+ Object topologyClasspathFirst = stormConf.get(Config.TOPOLOGY_CLASSPATH_FIRST);
+ List<String> firstClasspathList = new ArrayList<>();
+ if(topologyClasspathFirst instanceof List) {
+ firstClasspathList.addAll((List<String>)topologyClasspathFirst);
+ } else if (topologyClasspathFirst instanceof String) {
+ firstClasspathList.add((String) topologyClasspathFirst);
+ }
--- End diff --
You can add a log statement for firstClasspathList
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1507
@knusbaum Sorry I have been struggling to fix several bugs and another works. I'll have a look when I get some time.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69764532
--- Diff: storm-core/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -2104,6 +2109,17 @@ public String addToClasspathImpl(String classpath,
return StringUtils.join(l, CLASS_PATH_SEPARATOR);
}
+ public String addToClasspathImpl(Collection<String> classpath,
+ Collection<String> paths) {
+ String basePath = StringUtils.join(classpath, CLASS_PATH_SEPARATOR);
+ if(!basePath.equals("")) {
--- End diff --
`basePath` can be null when `classPath` is given as null. It is better to handle that scenario.
---
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 #1507: STORM-1916: Add ability for worker-first classpath
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1507#discussion_r69766341
--- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
@@ -1835,6 +1835,21 @@
public static final String TOPOLOGY_CLASSPATH="topology.classpath";
/**
+ * Topology-specific classpath for the worker child process. This will be *prepended* to
+ * the usual classpath, meaning it can override the Storm classpath. This is for debugging
+ * purposes, and is disabled by default. To allow topologies to be submitted with user-first
+ * classpaths, set the user.classpath.first.enabled config to true.
+ */
+ @isStringOrStringList
+ public static final String TOPOLOGY_CLASSPATH_FIRST="topology.classpath.first";
+
+ /**
+ * Enables user-first classpath. See topology.classpath.first
+ */
+ @isBoolean
+ public static final String STORM_USER_CLASSPATH_FIRST_ENABLED="storm.user.classpath.first.enabled";
--- End diff --
This can be `topology.classpath.first` or `pre.topology.classpath` with appending enabled which can be `topology.classpath.first.enabled` or `pre.topology.classpath.enabled`
---
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.
---