[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[revans2]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[asfgit]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[revans2]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[revans2]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[kishorvpatil]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: [STORM-1121] Remove method call to avoid overh...

[HeartSaVioR]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---