You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by weirdcanada <gi...@git.apache.org> on 2014/05/21 21:47:21 UTC
[GitHub] incubator-storm pull request: Testing: allow users to pass TEST-TI...
GitHub user weirdcanada opened a pull request:
https://github.com/apache/incubator-storm/pull/122
Testing: allow users to pass TEST-TIMEOUT-MS as param for complete-topology
It would be nice if `complete-topology` allowed a user to pass in the default timeout as a parameter (rather than just setting it as `5000ms`). I had a test that kept failing by taking too long.
This PR adds functionality without breaking any existing code. Tests pass.
(PS - not sure if this is the proper way to submit a PR for an apache project. I'm open to feedback. Thanks)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/weirdcanada/incubator-storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/122.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 #122
----
commit c86dbf956aabcd2623bdbc937dd51acfafe87d4a
Author: Aaron Levin <aa...@demeure.com>
Date: 2014-05-21T19:43:40Z
Allow users to pass TEST-TIMEOUT-MS as param
It would be nice if `complete-topology` allowed a usere to pass in
the default timeout as a parameter. This PR adds functionality without
breaking any existing code. Tests pass.
----
---
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] incubator-storm pull request: Testing: allow users to pass TEST-TI...
Posted by weirdcanada <gi...@git.apache.org>.
Github user weirdcanada commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-46256730
@revans2 I've created the JIRA ticket and updated the pull request. Let me take into account your feedback and re-submit a pull request. It may be a couple of days.
---
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] incubator-storm pull request: STORM-354: Testing: allow users to p...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-49430433
I am really sorry it has taken me so long to review things. The changes look good I am +1, I had to do an upmerge, because of whitespace issues due to a reformatting patch that went in previously.
I'll merge this into 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] incubator-storm pull request: STORM-354: Testing: allow users to p...
Posted by nsj007 <gi...@git.apache.org>.
Github user nsj007 commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-48413339
Is there any progress on this. The 5000ms timeout pretty much fails all my test when upgrading from STORM 0.9.01 to apache-storm-0.9.2-incubator
---
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] incubator-storm pull request: STORM-354: Testing: allow users to p...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/122
---
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] incubator-storm pull request: Testing: allow users to pass TEST-TI...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-45948782
@weirdcanada, any update on this PR?
---
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] incubator-storm pull request: Testing: allow users to pass TEST-TI...
Posted by weirdcanada <gi...@git.apache.org>.
Github user weirdcanada commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-46097193
I just got back from vacation last week. I'll submit the JIRA ticket and update the PR tomorrow (Sunday). 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] incubator-storm pull request: Testing: allow users to pass TEST-TI...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/122#issuecomment-45096868
All pull requests need a JIRA associated with them. Could you please file a JIRA if one does not already exist and update the title of this pull request to have the JIRA number in it, i.e. STORM-12345.
The code itself looks fine to me. But I would almost rather have a second defined timeout that is based off of the original timeout. Simply because in complete-topology simulate-wait within a loop. simulate-wait also has the same timeout code in it but in a loop 10 times, so if you get unlucky and get close to the timeout even once in simulate-wait the outer loop will timeout. We could simply have it be 3 * TEST-TIMEOUT-MS
---
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.
---