You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by csivaguru <gi...@git.apache.org> on 2016/05/23 19:52:26 UTC
[GitHub] storm pull request: STORM-1861 Fix storm script bug to not fork ja...
GitHub user csivaguru opened a pull request:
https://github.com/apache/storm/pull/1441
STORM-1861 Fix storm script bug to not fork java process
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/csivaguru/storm bug_fix/storm_script
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1441.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 #1441
----
commit 246ccd974826eccd4b7bc9aebd1be3dfcc04414e
Author: Sivaguru Kannan <sc...@hortonworks.com>
Date: 2016-05-23T19:50:48Z
STORM-1861 Fix storm script bug to not fork java process
----
---
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: STORM-1861 Fix storm script bug to not fork ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64634284
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
The goal was so that the code after this call would execute.
```
os.remove(tmpjar)
```
Without that we leak a file.
---
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: STORM-1861 Fix storm script bug to not fork ja...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64637467
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
Ah... that makes sense now. So how concerned are we about leaking a tmp file vs. allowing users to build tools around topology submission (and get the right exit code)?
Would it make sense to have a separate command to do the topology transform instead of in-lining it with topology submission? I.e. two steps: 1. transform topology, 2. submit topology jar output from step 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: STORM-1861 Fix storm script bug to not fork ja...
Posted by csivaguru <gi...@git.apache.org>.
Github user csivaguru closed the pull request at:
https://github.com/apache/storm/pull/1441
---
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: STORM-1861 Fix storm script bug to not fork ja...
Posted by csivaguru <gi...@git.apache.org>.
Github user csivaguru commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64669384
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
@revans2 I was planning to update this patch with the same approach (save the exit code and return it after deleting the tmpjar)
---
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: STORM-1861 Fix storm script bug to not fork ja...
Posted by csivaguru <gi...@git.apache.org>.
Github user csivaguru commented on the pull request:
https://github.com/apache/storm/pull/1441#issuecomment-222000942
@harshach, @revans2, @ptgoetz, @hmcl I am abandoning the PR in favor of https://github.com/apache/storm/pull/1448 which I made against the right branch, 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: STORM-1861 Fix storm script bug to not fork ja...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64598586
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
@hmcl I don't see a reason to fork the submission process.
I'm +1 for the patch.
---
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: STORM-1861 Fix storm script bug to not fork ja...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64300845
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
@revans2 can you please take a look and let us know if there is any special reason for this process to be forked? This process being forked affects the return code. If there is no reason for this process to be forked, this fix will correct the return code. 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: STORM-1861 Fix storm script bug to not fork ja...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64604305
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
+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: STORM-1861 Fix storm script bug to not fork ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1441#discussion_r64638916
--- Diff: bin/storm.py ---
@@ -241,7 +240,6 @@ def jar(jarfile, klass, *args):
extrajars=[tmpjar, USER_CONF_DIR, STORM_BIN_DIR],
args=args,
daemon=False,
--- End diff --
This was supposed to make it transparent to the end user.
Why can't we return the exit code save it and then return it after the delete?
---
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.
---