You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by javrasya <gi...@git.apache.org> on 2014/11/18 15:24:55 UTC
[GitHub] storm pull request: Custom TEST-TIMEOUT-MS Definition Method Chang...
GitHub user javrasya opened a pull request:
https://github.com/apache/storm/pull/316
Custom TEST-TIMEOUT-MS Definition Method Changed
Custom test timeout with variable TEST-TIMEOUT-MS was being defined with System Environment Variable. This cause a problem to define it on every test simply. System property is now being used to define custom TEST-TIMEOUT-MS variable value. This can be simply set before every test as
System.setProperty("STORM_TEST_TIMEOUT_MS", "20000");
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/javrasya/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/316.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 #316
----
commit f935fde4e2eebfea6f69906694c5bbb273f5a184
Author: Ahmet DAL <ce...@gmail.com>
Date: 2014-11-18T14:24:17Z
Custom TEST-TIMEOUT-MS Definition Method Changed
Custom test timeout with variable TEST-TIMEOUT-MS was being defined with System Environment Variable. This cause a problem to define it on every test simply. System property is now being used to define custom TEST-TIMEOUT-MS variable value. This can be simply set before every test as
System.setProperty("STORM_TEST_TIMEOUT_MS", "20000");
----
---
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: Custom TEST-TIMEOUT-MS Definition Method Chang...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-63937307
Time out should be set simply. This doesn't just depends on machine. My spouts or bolts would have needed some time to sleep or to be done. So it also depends on complexity. You are right, It is mostly about machine but I think, at least, both should be supported.
---
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-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64191337
@HeartSaVioR you are right. This is better solution to add that parameter to `complete-topology` in testing4j.clj. I added this capability to the function but not committed yet. I couldn't find any task and I opened one in JIRA. I'll make new 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] storm pull request: [STORM-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-63993272
@javrasya OK. I think it would be better than current.
Btw, PR seems to need to change, to reflect modified objective.
---
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: Custom TEST-TIMEOUT-MS Definition Method Chang...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-63976738
I think, choosing higher one would be necessary solution as you mentioned. Actually, it is needed to set that value simply in code somehow. I think there is no need more complex development like multiplier etc.
I will change the request 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 pull request: [STORM-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya closed the pull request at:
https://github.com/apache/storm/pull/316
---
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-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64126434
Changes is 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 pull request: [STORM-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64136784
@harshach Yes, you're right. I didn't intend it.
Btw, how about using higher value between system environment and each unit test's custom timeout?
@javrasha I thought you saw STORM-492 carefully, but you didn't.
To achieve setting up each test's timeout, it shouldn't rely on system property, it should be done with wait-until-cluster-waiting's parameter, and pick higher value.
---
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-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64175854
@javrasya I added trackedWait to have timeout-ms as parameter, but not completeTopology.
Then we should add a parameter to completeTopology & complete-topology to make it work.
In JVM we can pass a parameter. If it doesn't have parameter, I think it's better to let them have 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: [STORM-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64136825
@javrasya mistakes on mentioning.
---
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: Custom TEST-TIMEOUT-MS Definition Method Chang...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-63941014
You're right. It also depends on complexity.
But Storm unit test cannot use both timeout configuration, it should choose one.
I don't know which one it should choose.
In addition, longer timeout (based on experiment and expectation) with complex unit test cannot cover "more slower" machine.
Maybe configuring timeout multiplier could help.
---
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-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64132928
@javrasya @HeartSaVioR I don't understand this change. We already discussed this in another JIRA that having systemProperty doesn't pass in the variable to testing.clj. I tested this PR with following command
"mvn clean install -DstormTestTimeoutMs=20000"
and my test timeout still 5000.
If the intention of this patch is for other tests to increase the timeout they can pass it as parameter to
(defn wait-until-cluster-waiting
"Wait until the cluster is idle. Should be used with time simulation."
([cluster-map] (wait-until-cluster-waiting cluster-map TEST-TIMEOUT-MS))
([cluster-map 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.
---
[GitHub] storm pull request: [STORM-492] Custom TEST-TIMEOUT-MS Definition ...
Posted by javrasya <gi...@git.apache.org>.
Github user javrasya commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-64173340
@harshach , change is actually because of setting that timeout value in code simply. Environment variable can not be set in code. But property can be set.
```
System.setProperty("stormTestTimeoutMs","5001")
```
It was not for customising that value on console which is passing `-DstormTestTimeoutMs=5001` parameter.
Are you sure, you tested it well. Cause I tried it and I saw the timeout is changed to 5001. Here is my test;
```
$ mvn -Dtest=net.egemsoft.lol.engine.storm.MainFlowTest#testMainFlow test
.....
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 14.044 sec <<< FAILURE! - in net.egemsoft.lol.engine.storm.MainFlowTest
testMainFlow(net.egemsoft.lol.engine.storm.MainFlowTest) Time elapsed: 13.934 sec <<< FAILURE!
java.lang.AssertionError: Test timed out (5001ms) (not (every? exhausted? (spout-objects spouts)))
at backtype.storm.testing$complete_topology.doInvoke(testing.clj:501)
at clojure.lang.RestFn.invoke(RestFn.java:826)
at backtype.storm.testing4j$_completeTopology.invoke(testing4j.clj:61)
at backtype.storm.Testing.completeTopology(Unknown Source)
at net.egemsoft.lol.engine.storm.MainFlowTest$1.run(MainFlowTest.java:147)
at backtype.storm.testing4j$_withSimulatedTimeLocalCluster$fn__9107.invoke(testing4j.clj:93)
at backtype.storm.testing4j$_withSimulatedTimeLocalCluster.invoke(testing4j.clj:93)
at backtype.storm.Testing.withSimulatedTimeLocalCluster(Unknown Source)
at net.egemsoft.lol.engine.storm.MainFlowTest.testMainFlow(MainFlowTest.java:77)
Results :
Failed tests:
MainFlowTest.testMainFlow:77 Test timed out (5001ms) (not (every? exhausted? (spout-objects spouts)))
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
```
@HeartSaVioR, my project is in Java. I could not access `wait-until-cluster-waiting` function. Because it is in `testing.clj` and my project is probably using `testing4j.clj`. I looked to all code in `testing4j.clj` there is no access to `wait-until-cluster-waiting`. I think there is no need to access that function neihter.
I invoke `complete-topology`. I see the function takes `timeout-ms` parameter. It can be given as parameter. But as I mentioned above, I am using testing4j.clj result and the `complete-topology` function in `testing4j.clj`, does not accept `timeout-ms` parameter. Please check it in `testing4j.clj`.
So there is only way left I can see, `TEST_TIMEOUT_MS` variable should also be fed by system property.
---
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: Custom TEST-TIMEOUT-MS Definition Method Chang...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/316#issuecomment-63486189
Hello.
To understand the reason, please reference STORM-492.
Actually test time is affected to user machine rather than test complexity,
so fixed timeout value is unusable.
On 2014년 11월 18일 (화) at 오후 11:24 Ahmet DAL <no...@github.com> wrote:
> Custom test timeout with variable TEST-TIMEOUT-MS was being defined with
> System Environment Variable. This cause a problem to define it on every
> test simply. System property is now being used to define custom
> TEST-TIMEOUT-MS variable value. This can be simply set before every test as
>
> System.setProperty("STORM_TEST_TIMEOUT_MS", "20000");
> ------------------------------
> You can merge this Pull Request by running
>
> git pull https://github.com/javrasya/storm master
>
> Or view, comment on, or merge it at:
>
> https://github.com/apache/storm/pull/316
> Commit Summary
>
> - Custom TEST-TIMEOUT-MS Definition Method Changed
>
> File Changes
>
> - *M* storm-core/src/clj/backtype/storm/testing.clj
> <https://github.com/apache/storm/pull/316/files#diff-0> (2)
>
> Patch Links:
>
> - https://github.com/apache/storm/pull/316.patch
> - https://github.com/apache/storm/pull/316.diff
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/storm/pull/316>.
>
---
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.
---