You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2015/10/21 19:37:38 UTC
[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/810
[STORM-1121] Remove method call to avoid overhead during topology submission time
Nimbus calls mk-assignments from SubmitTopology within lock is causing it to wait for processing of all heartbeats. This conflicts with recurring mk-assignments call. Topology can be submitted without making assignments, since for new topology the assignments would be made available as part of next scheduling cycle.). This gives more consistent response time as avoid locking within nimbus.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm STORM-1121
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/810.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 #810
----
commit 1593a374a4a381238f39fd9b0d078586d1aaa305
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2015-10-19T23:32:18Z
Remove method call to avoid overhead during topology submission 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: [STORM-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150049582
@HeartSaVioR looking into compilation issue. Somehow it did not get committed. Re-running the build.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150712631
@kishorvpatil
No, just marking ```nimbus.reassign``` option as deprecate and leave proper reason from 0.10.x would be fine.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150045478
@kishorvpatil
You may want to check Travis build failure, seems like there's a missing spot.
> Exception in thread "main" java.lang.IllegalArgumentException: Unable to resolve classname: RebalanceOptions, compiling:(backtype/storm/transactional_test.clj:417:26)
---
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-1121] Remove method call to avoid overh...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150671588
The changes look good I am +1, and time travis took is about the same as before.
---
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-1121] Remove method call to avoid overh...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-149996906
I love the concept, but I would like to see the tests updated so we are not adding several mins to the time it takes to run the unit tests.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150705382
@kishorvpatil
With this change users have no option to turn off reassign since it makes cluster unusable (cluster cannot run topology unless user calls other operation which triggers mk-assignments) , so I think it should be not selectable to users.
If we need ```nimbus.reassign``` just for testing, we should deprecate this option to 0.10.x and hide this option.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150051486
@kishorvpatil
rebalance seems not work cause it requires topology to be alive at the moment.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150697902
@kishorvpatil
Test changes look good.
@kishorvpatil @revans2
I'd like to see my previous comment (https://github.com/apache/storm/pull/810#issuecomment-150047429) makes sense, and if it does, I think we should address these things.
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150667453
@HeartSaVioR Thank you for review and suggestions. I have made changes so that our tests are using simulated-time cluster to avoid `Thread/sleep`. Please revisit the code and let me know if you have any further suggestions/questions.
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150047429
I also love the concept.
Btw, in order to rely on recurring mk-assignments,
- We should remove ```nimbus.reassign``` since it should be true for new topologies to be assigned. We can't change ```when statement``` cause it makes nimbus.reassign ineffective.
- It would be better to let users know that new topology will be assigned by nimbus not at the moment but within ```nimbus.monitor.freq.secs```.
```
(schedule-recurring (:timer nimbus)
0
(conf NIMBUS-MONITOR-FREQ-SECS)
(fn []
(when (conf NIMBUS-REASSIGN)
(locking (:submit-lock nimbus)
(mk-assignments nimbus)))
(do-cleanup nimbus)
))
```
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150730065
@kishorvpatil Amazing! +1. Test failure is unrelated. (Kafka)
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150699962
@HeartSaVioR I tried that for supervisor_test.clj, somehow mocking of submit topologies etc.methods don't make it easy. It seems straight forward to rebalance and proceed.( usually because moving time forward will also require you to send all necessary heartbeats etc.)
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150703213
@HeartSaVioR I don't think we can remove `NIMBUS-REASSIGN` as it used mostly during supervisor_test to avoid rescheduling with value `false`. The documentation clearly advises users to not set this value to `false`.
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150729673
@HeartSaVioR
- Modified to make "nimbus.reassign" unavailable for users.
- Created #815 to decommission variable for 0.10.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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150699176
@kishorvpatil
Seems like some places in the tests still rely on rebalance. Could we change these to use simulated-time cluster as well when possible?
---
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-1121] Remove method call to avoid overh...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150701869
@kishorvpatil
Good, I didn't think about mocking. Your additional changes look great.
Just I wish to discuss how we can do with ```nimbus.reassign```.
If my understand is right, what if user turns off ```nimbus.reassign``` so that recurring is not in place?
And maybe minor but what if user set ```nimbus.monitor.freq.secs``` a bit higher so that recurring interval is also longer?
---
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-1121] Remove method call to avoid overh...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/810#discussion_r42666392
--- Diff: storm-core/test/clj/backtype/storm/integration_test.clj ---
@@ -236,6 +237,7 @@
"acking-test1"
{}
(:topology tracked))
+ (Thread/sleep 11000)
--- End diff --
Is there a way we can force mk-assignments to be called? I don't really like adding around 1 min to the time it takes to run the tests needlessly.
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150702270
@HeartSaVioR Removed rebalance calls at places where I could use simulated time cluster.
---
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-1121] Remove method call to avoid overh...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/810#issuecomment-150711148
@HeartSaVioR I complete agree with decommissioning this configuration used only for testing. It should be unavailable for users. Let me deprecate this variable and add documentation.
Are you suggesting we push this change to 0.10.x 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: [STORM-1121] Remove method call to avoid overh...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/810
---
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.
---