You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by roshannaik <gi...@git.apache.org> on 2016/03/16 05:35:27 UTC
[GitHub] storm pull request: STORM-1632 Disable event logging by default
GitHub user roshannaik opened a pull request:
https://github.com/apache/storm/pull/1217
STORM-1632 Disable event logging by default
updating setting in defaults.yaml
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/roshannaik/storm STORM-1632
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1217.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 #1217
----
commit a33bc455db2c4cce0d355c9c0fa4da60471b859b
Author: Roshan Naik <ro...@hortonworks.com>
Date: 2016-03-16T04:26:12Z
STORM-1632) Disable event logging by default
----
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204239172
@roshannaik thanks for fixing it in the right way. I have run some quick tests and patch it works as expected.
+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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200233334
Re-ran the exclamation topology with,
- no anchoring/acking
- TestWordSpout emitting a fixed word without any sleep
- spout, bolt parallelism = 1
- worker = 1
- Loglevel = error, conf.setDebug(false)
Throughput I observed :-
566 K tuples/sec (with eventlogger: nil)
569 K tuples/sec (with eventlogger: 0) roughly 0.5% improvement.
I then ran Jprofiler and saw **1.3%** time being spent in send_to_eventlogger.
The profiler output itself might be offset due to the instrumentation overhead. Jprofiler detects the following with send_to_eventlogger
`some methods with excessive instrumentation overhead has been detected. They are called very frequently, their execution times are very short and the time required for measuring those calls are dispropotional`
>I retired the throughput measurements with ackers=0 .. impact is even greater ... its 25% faster when event.logger=0
Are you taking the measurements while the profiler is running or with the debug flag turned on? I don't see this happening otherwise. Are you using the latest Jprofiler (9.1.1) ? Are you using any extra plugins to instrument the hashmap lookups (since I see the hashmap keys in your screenshot) ? If so that itself might be skewing your results.
To avoid the persistentMap lookups, I also tried passing the **storm-id** and **debug-atom** values as args to send_to_eventlogger and the percentage reduced from 1.3 % to 1 % . You could try this change and see how it impacts your topology.
I agree with @HeartSaVioR and don't think we need to set eventlogger to 0.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200137195
I retired the throughput measurements with ackers=0 .. impact is even greater ... its **30%** faster when event.logger=0
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-203691155
@roshannaik, thank you for your patience, persistence, and dedication throughout the lifecycle of this patch. I hate to vote -1 on anything. I'm really glad we're working toward a solution that everyone supports.
I have yet to review and test, but definitely plan to approve if all works out.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201716320
@arunmahadevan
Please post pull request with your patch.
I tried to change the way to update debug information from executor by pushing system tuple, and it increases throughput for EL=1, but it also decreases throughput for EL=0 compared to before.
I'll try to improve my approach if your patch doesn't help much, but according to your number it seems to end up this long conversation.
@roshannaik
Could you please apply @arunmahadevan patch and do benchmarks again? Thanks in advance!
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/1217#discussion_r58577654
--- Diff: storm-core/src/ui/public/js/script.js ---
@@ -220,19 +220,21 @@ function topologyActionJson(id, encodedId, name, status, msgTimeout, debug, samp
jsonData["deactivateStatus"] = (status === "ACTIVE") ? "enabled" : "disabled";
jsonData["rebalanceStatus"] = (status === "ACTIVE" || status === "INACTIVE" ) ? "enabled" : "disabled";
jsonData["killStatus"] = (status !== "KILLED") ? "enabled" : "disabled";
- jsonData["startDebugStatus"] = (status === "ACTIVE" && !debug) ? "enabled" : "disabled";
+ jsonData["startDebugStatus"] = (status === "ACTIVE" && loggersTotal!=null && loggersTotal!=0 && !debug) ? "enabled" : "disabled";
--- End diff --
Good catch. I think we should remove `loggersTotal != null` so that "debug" is enabled if the value is set to `null` since one event logger task is created per worker if the value is set to null. This interpretation of "null" is not very intuitive, but its consistent with what "null" means with other variables like "topology.acker.executors".
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-199583523
@arunmahadevan I think your Visuam VM output is not presenting the data in a manner you are expecting. Here is the JProfiler output.:
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200571684
@roshannaik
I'm sorry I feel I picked the wrong word `micro optimization` to confuse you what I mean. `local optimization` seems clearer.
Btw, I guess @ptgoetz got me.
Yes, I agree we need micro-benchmark to clear out variables, but I think it has to be re-evaluated with normal benchmark to reason how it affects in relatively normal situation, especially if it has to touch functionalities.
If it doesn't touch functionality I would say "Awesome work!" even though under 5% of performance gain on local optimization.
(Why STORM-1526 and STORM-1539 didn't need to re-evaluate with normal benchmark is that it didn't affect any functionalities.)
And I guess this overhead (0.006720889 ms = 6720.889 ns per each tuple spend in send_to_eventlogger as @arunmahadevan posted) is relatively very small than what Storm has to do for process tuple - enqueue and dequeue, finding task id to send, serde, transfer - which we may find spots to improve.
Though I agree that's inside of critical path so we may want to find the alternative way with not touching functionality.
If you really want to disable it by default, it would be better to post mail to dev mailing list to build consensus first.
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-203792662
1 . The tooltip takes some time to show up, whereas the tooltip on other elements like the title elements under Topology summary shows up immediately. I played around a bit and found a way to show the tooltip on disabled button immediately using the same style as other tooltips. You need to wrap the input element in a span like below,
```javascript
<span style="display:inline-block;" data-toggle="tooltip" title="" data-original-title="To debug set topology.eventlogger.executors to a value > 0 or nil">
<input disabled="" onclick="confirmAction('test-topology-1-1459402415', 'test-topology', 'debug/enable', true, 10, 'sampling percentage', 'debug')" type="button" value="Debug" class="btn btn-default">
</span>
```
2 . The tooltip is shown even if the button is enabled. You could add the `<span> </span>` only when number of executors is 0, which would take care of this.
3 . The tooltip is also shown when the debug button gets disabled after the user clicks on it (when topology.eventlogger.executors > 0). Similar fix as above.
4 . The button should be disabled in the spout/bolt pages 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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204524132
Thanks to each of you for lending your time and effort with this.
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-199235692
@roshannaik profiled "org.apache.storm.starter.ExclamationTopology" after setting log level to ERROR and the `send_to_eventlogger` doesn't show up in the top 25 methods in terms of CPU time. Tried with different configurations (one worker, three workers, increased and reduced the spout and bolt parallelism) but the results were more or less the same.
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197707627
@roshannaik Thanks for the details. I will try and run a similar topology to see the difference and see if there is something we can do. Do you have any suggestions on how we can improve the performance of the check in clojure ?
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-202538771
@HeartSaVioR i think there is only so much effort I can keep putting into providing credible evidence that there is an issue or hand holding other people. IMO there is enough of already shared here. I prefer not to try to convince people against their own will.
Like i mentioned before, @arunmahadevan 's theory is incorrect. On a side note, any spout that generates data from will do something similar.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200918016
@arunmahadevan , like i already said earlier, i did **not** take the throughput measurements with the profiler attached. it makes no sense to do so. profiling was done separately.. and it correlated with the throughput measurements.
---
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-1632 Disable event logging by default
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204220646
+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-1632 Disable event logging by default
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1217
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-202658895
While I'd like to merge @arunmahadevan patch since it just improves performance with no change on functionality, I'm also with @ptgoetz's suggestion.
Since title of the issue is 'Disable event logging by default', I think we can address performance improvement to other issue and handle separately.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201074919
Rewrote the topology so its easier for you folks to run.
Uploaded into the jira and provided instructions here : https://issues.apache.org/jira/browse/STORM-1632?focusedCommentId=15211001&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15211001
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-199754698
@roshannaik I ran the Exclamation topology with the TestWordSpout emitting a tuple every 10 ms with a spout parallelism of 10 and measured the throughput with and without event logger and I observed almost the same results.
I am also attaching the Jprofiler output after tracing the topology for about 1.36 Million tuples where the send_to_eventlogger is consuming only around 0.2% time.
<img width="1220" alt="spout" src="https://cloud.githubusercontent.com/assets/6792890/13949426/fac1a5ee-f04a-11e5-95f1-f5d9a412ff77.png">
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-203788623
@roshannaik
Thanks for your quick reaction.
I applied your patch and verified that 1. topology.eventlogger.executors is 0 at default 2. tooltip is shown when 'Debug' button is disabled, and it describes how to enable event logger.
<img width="1106" alt="topology-eventlogger-tooltip-works" src="https://cloud.githubusercontent.com/assets/1317309/14167650/adbd7188-f757-11e5-95ae-5950777c4f08.png">
I'm also talking a look into #1272, but it's for improving performance when topology event logger > 0, not for blocking this.
Thanks again for your patience. +1 overall for me.
---
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-1632 Disable event logging by default
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197700388
Thanks @roshannaik for detailed explanation. I wasn't expecting this. We should fix the perf hit in the check.
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197523828
@roshannaik setting `topology.eventlogger.executors: 0` completely disables event logging by default (i.e. no threads are started to handle event logging). If someone tries to turn on event logging in the topology, they wont see any events logged and this is a very confusing behavior in my opinion. It makes them think that this functionality is totally broken or something is wrong with their topology.
The event logging happens only when they enable it and we don't expect them to turn it on all the time. The actual overhead otherwise is [to check if a few flags are turned on](https://github.com/apache/storm/blob/1.x-branch/storm-core/src/clj/org/apache/storm/daemon/executor.clj#L480).
Can you post more details on the topology you ran and the results you observed when you set topology.eventlogger.executors to 0 vs setting it to `nil` ?
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-202556630
It should be fairly easy to alter the UI to make it clear to users when this is disabled, as well as how to enable it. If we can do that I would support disabling it by default (i.e. I would be +1).
My main objection with this patch is that will make the UI appear broken by default and confuse users, especially new users.
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201412463
With the BasicTopology.java uploaded in the Jira I was not getting consistent results and in some cases the throughput without event logger turned out to be less. After going through the spout it appears that with every emit a new Long is created.
`collector.emit(new Values(new Long(rand.nextInt(Integer.MAX_VALUE))), messageCount);`
With the spout emitting 400+ Million tuples in 10 mins (the new Long alone eating up 3.2 GB memory), I thought the GC could have distorted the results. So I changed the spout to emit a fixed long value and measured the throughput and heres what I observed.
[No eventlogger](https://cloud.githubusercontent.com/assets/6792890/14049885/1ccdc2ae-f2de-11e5-9f50-231990d629bb.png) 753394 tuples/sec
[With eventlogger](https://cloud.githubusercontent.com/assets/6792890/14049918/4dd0a902-f2de-11e5-8a77-5c6ba3f20520.png) 736517 tuples/sec (2.29 % hit)
[Eventlogger with the patch](https://cloud.githubusercontent.com/assets/6792890/14049934/61fbdce4-f2de-11e5-8454-e27db755fdc3.png) 749492 tuples/sec (0.52 % hit)
The results with the [patch] (https://github.com/arunmahadevan/storm/commit/7eae5ec9f63cee82c49980a3bedf5f0dfe4e3a8d) to reduce the PersistentMap lookup and the modified [BasicTopology](https://github.com/apache/storm/files/189869/BasicTopology.txt) can be validated and if that overhead is not acceptable, we can disable the "debug" button by default and show a tooltip on how to enable 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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200677018
@harshach I had removed the sleep in the latest run to match what @roshannaik was evaluating.
---
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-1632 Disable event logging by default
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197534417
I agree with @arunmahadevan. You can also show an alert message in UI, when user turns on the event logging for a topology e.g. "Event logging may degrade the performance" or something similar.
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200559629
I did a quick benchmark on a real cluster (albeit on a VMware cluster) and found that there was a throughput hit, but it was small -- about 0.4%.
I'm okay with leaving the defaults as is, and documenting how to disable the feature.
If there's a better solution, I'm okay with waiting for a post 1.0 release.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201116132
@ptgoetz I understand it is not hard to change the config. But the default behavior shouldn't be affecting peformance. Going to 1.0 can hit 9% perf hit just because we are shipping a new feature that can cause performance issue . UI can be fixed in the same patch as well. Just don't show the buttons on the UI if the event.loggers are set to 0. Its a SIMPLE FIX
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204224677
Thanks for the last-minute effort @roshannaik.
+1
I typically like to wait for 24 hrs. after the last commit on a PR to merge, even though it's not required by our bylaws.
I intend to merge this earlier in the interest of getting the 1.0 release out. If there are any objections after the fact, there will plenty of time to cancel the VOTE, revert, 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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200136661
@arunmahadevan
Your call tree seems a bit different than mine.
I dont see why you would set a 10ms wait.. your throughput computations will be naturally thrown off by that.
Also set spout parallelism = 1 and bolt =1, worker=1 when profiling.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200143091
In my opinion, since this performance hit can be quite large, we should definitely set eventLoggers=0 by default, and additionally the UI should also disable the 'debug' button if eventLoggers=0
That will avoid the confusion to users that Arun was mentioning earlier... This feature has some needless penalties and wasting CPU cycles even when not in use.
I tried switching the clojure lookups into a java implementation .. but that didnt help. So i don't see a way to speed up these lookups.
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201120771
@hararha Keep in mind that anyone packaging Apache Storm is free to make changes.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201632410
@harshach my numbers for this rewritten topology were ..
**EL=1, Acker=0 :** 641,202 acks/sec, 1,282,413 emits/sec
**EL=0 , Acker=0 :** 719,399 acks/sec, 1,438,798 emits/sec
Approx 12% improvement over EL=0
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-206117168
@roshannaik yes, it will make the behavior consistent with "ackers". You may also want to update the tooltip. Since this PR is already merged, you may have to raise another one and not sure if that would make it to 1.0 release.
---
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-1632 Disable event logging by default
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204225005
@ptgoetz +1 for quick merge
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200629951
Perhaps we can wait a bit before concluding on this. Some of you feel confident that there is a minor or no hit based on whatever topology/setup you have used. Whereas it is quite significant on the one i used.
My current topology is implemented to run within the storm-benchmark tool. I can rewrite that as a regular topology so you can run it easily to validate.. OR, if you are comfortable running it, i can share it as is more quickly.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201637760
@HeartSaVioR the topology is internally overriding some of those values like worker count. You will need to comment it out if you'd like to try varying them from cmd line.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201117315
@ptgoetz whats the timeline 1.0 release lets put up a date I am willing to push a patch for this tonight on the UI side. If thats what blocking the release.
---
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-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200591500
We're debating six versus one half dozen. Do we disable it by default and explicitly tell users they have to turn it on for the UI functionality to work? Or do we enable it by default and tell users to disable it per topology to realize a small performance gain?
I could go either way, but the latter seems like a better user experience for users new to the feature.
Also, the minor performance hit is eclipsed by the performance improvements in 1.0. And it can be easily turned off. It just needs to be documented clearly, IMO.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197551650
@abhishekagarwal87
This perf hit of 7-9% exists even if user does not enable logging from the UI. With logging enabled, it should be even higher.
@arunmahadevan
The perf improvement i described here is observed when going from the default value nil to 0 for topology.eventlogger.executors
**Topology detail:** 1 spout, 1 bolt, 1 acker. Spout generates random numbers. Bolt does this math on the tuple: (value+1)*2. In effect this is just a speed of light topology.
The perf hit that i noted is actually due to those very same checking of flag.
This checking of flags in clojure turns into very expensive lookups in clojure internals. It internally invokes Java reflection! If you are thinking 'what the hell'.. then yes that was my reaction too.
Specifically this is the problematic lookup for this code path : 'storm-component->debug-atom'
I agree with @arunmahadevan 's concern that this will confuse the users when they don't see logs after enabling it on the UI.
The alternative fix for this is to change the manner in which this flag is made available to the code. Basically make it more efficient.
There are some other lookups in the critical path that are also are causing perf hits... which i plan to address in a separate jira.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201122231
Just to update, here's my numbers.
```
> -c topology.eventlogger.executors=0 -c topology.max.spout.pending=2000 -c topology.disruptor.batch.size=10000
10m 0s 1001252900 500626839 0.000 500626428
10m 0s 1031746043 515873225 0.000 515872910
> -c topology.eventlogger.executors=1 -c topology.max.spout.pending=2000 -c topology.disruptor.batch.size=10000
10m 0s 933932980 466965808 0.000 466965793
10m 0s 922674155 461337280 0.000 461337282
----
> -c topology.eventlogger.executors=0 -c topology.max.spout.pending=2000 -c topology.disruptor.batch.size=10000 -c topology.workers=2
10m 0s 985430460 492607760 0.000 492607800
10m 0s 1012353360 506175040 0.000 506175060
> -c topology.eventlogger.executors=1 -c topology.max.spout.pending=2000 -c topology.disruptor.batch.size=10000 -c topology.workers=2
10m 0s 945182034 472570221 0.000 472570221
10m 0s 919781311 459891331 0.000 459891312
```
Performance hit is shown, and it seems that it even affects spout and bolt are running in different workers.
Since I ran test with my local machine so it could be flaky. Anyway it seems clear that there's performance hit.
If we can test it into multiple machines that would be good.
**But anyway, I really don't want to drag releasing 1.0.0 so we want to keep it as is in 1.0.0, I'm OK.**
No need to vote -1 to this PR but let's change priority on issue to 'Critical' or even 'Major', and remove Epic link.
There're some issues still open with Epic, so we may have to move to resolve them ASAP.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201079820
@HeartSaVioR @arunmahadevan @ptgoetz ran the topology from @roshannaik and these are the numbers I got on a single node cluster with single worker.
with event-loggers set to 0
668748 tuples per sec
with event-loggers set to 1
609700 tuples per sec
A difference of 59048 tuples per sec and 8.82% thoughput affect. Still a considerable amount of % impact to consider. I'll wait for other 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-1632 Disable event logging by default
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197201727
The actual event logging happens only if user starts it from the topology page. Only then performance degradation will happen. isn't 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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-205985587
On further thought... i feel it maybe ok to just remove the loggersTotal != null check here. Shall I just update 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] storm pull request: STORM-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201119172
Just to break the stalemate here, I am doing +1 on going on with 1.0 release without this patch. Given that patch is open for 9 days we should be better about addressing and reviewing important patches like these instead of going back'n forth without making much progress.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204198533
Update: Have fixes for pts 1 2,3 & 4 raised by Arun.
To support the changes in the component level page would have required a change the thrift spec to add the additional info in the response object. However thanks to help for @harshach was able to find an workaround to avoid that.
Now the tooltips are shown if both these conditions are met: debug button is disabled and the event loggers are disabled.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-204101205
1. I spent sometime trying to see if the tooltip can show up faster. But there is a catch-22 there. The Bootstrap based tooltip mechanism allows it, but the Bootstrap based tooltips wont show up if button is disabled. So had to settle for this mechanism... which has a downside that it doesnt allow customization of color and timing to the best i could check. The solution you describe here is among one of the alternative I had already tried and it doesn't work. The tooltip wont show up unless you hover the mouse over the boundary between the enclosing span element and the disabled button.
2. & 3. Although nice to not show tooltip when enabled, I thought its OK to show the tooltip in both enabled/disabled states of the button. Given the desire for quick turn around, i felt it was wise to not pursue implementing/testing this nice to have, but non-critical feature.
4. Let me look into 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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-202588189
@ptgoetz , i fully support that line of thought.
I can take a stab at making that UI change and including that fix in 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] storm pull request: STORM-1632 Disable event logging by default
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201105956
Yes, there is a performance hit. Especially in a single node cluster (don't know if you were running multiple workers), and networking doesn't come into play . I'm thinking of the big picture where you have multiple nodes. Then the numbers change, and it's not that big a hit.
If we release with what we have now, I would leave things as is, and document that there's a performance benefit to turning it off. It's not hard to twiddle that flag.
If we release as is, with this patch, the ui won't work as expected. Users will be confused, and anyone in a support role (I.e. Us) will be barraged with questions. And users will see a broken product.
The performance hit here is also minor compared to the performance gain from simply upgrading to Krio 3.
I think this is an important issue to address, but I don't think it's that big a deal that it should block the 1.0 release. We can address it better later in a minor release.
Let's get 1.0 out the door.
-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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200669173
@roshannaik did you try passing the map values are arguments and take the measurements? Based on your earlier results it appeared that PersistentMap lookup was causing the hit (I still think it could very well be due to the profiler overhead)
Here are the changes I made - https://github.com/arunmahadevan/storm/commit/7eae5ec9f63cee82c49980a3bedf5f0dfe4e3a8d . I would like see how it affects your profiling.
I don't think a 0.4 to 0.5 % increase in throughput should be a reason to completely disable a feature. And spouts that emit tuples in a tight loop would not be a very common use case whatsoever. I am for documenting this feature so that the users can adjust the config values based on their needs rather than turning it off.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200231974
@roshannaik
I feel this is similar to #1242. The difference is that #1242 is related to nextTuple call count, and this is related to emit tuple count.
Similar to #1242, I think `turning on/off debug` is not that critical so that we check for every emits.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-206553359
@abhishekagarwal87 @arunmahadevan
Since it wont make it into 1.0.. i shall make this change part of a separate jira & PR. That will avoid confusion.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200662946
OK I'm assuming this is valid performance hit whether it is small or huge.
For choosing 6 or half a dozen, I think whether turning on or off by default should be decided on that its use cases are valid for production.
Since STORM-954 is created by @harshach, I guess he considered use cases for this. If we predict most use cases are in dev., sure I agree we can just disable it by default, and document how to enable.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201689685
@roshannaik
Command line options override existing configurations.
https://github.com/apache/storm/blob/master/storm-core/src/jvm/org/apache/storm/utils/Utils.java#L452
```
895 [main] INFO o.a.s.StormSubmitter - Submitting topology BasicTopology in distributed mode with conf {"topology.acker.executors":0,"storm.zookeeper.topology.auth.scheme":"digest","topology.workers":2,"topology.debug":false,"storm.zookeeper.topology.auth.payload":"-6295626850175862189:-8269401123970688630","topology.disruptor.batch.size":10000,"topology.eventlogger.executors":0,"topology.max.task.parallelism":1}
1118 [main] INFO o.a.s.StormSubmitter - Finished submitting topology: BasicTopology
```
---
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-1632 Disable event logging by default
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200672407
@harshach
> @arunmahadevan @HeartSaVioR Looking at the perf analysis from @roshannaik It looks to be there is enough evidence to consider this as serious issue in performance.
Can you also take a look at the results that I observed where the throughput difference is negligible ? I am for disabling it if theres a consensus on the results and that it really affects performance.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200674706
@arunmahadevan I saw the earlier comment. Is that topology ran with 10ms sleep in the spout?
@roshannaik do you also have some numbers like Arun posted , events per sec.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200553473
@arunmahadevan @HeartSaVioR Looking at the perf analysis from @roshannaik It looks to be there is enough evidence to consider this as serious issue in performance. Given that eventlogging is new feature and we do have evidence its causing perf issue I am +1 on disabling it by default. I understand that once they disabled they can't enable it in a running topology and that is OK. For most usecases this might be used in dev cluster than a production cluster. Also this is a blocker for 1.0 release , lets get this merged in and see if there is a better a way to enable it by default and we can that in 1.1 release.
---
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-1632 Disable event logging by default
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1217#discussion_r58558813
--- Diff: storm-core/src/ui/public/js/script.js ---
@@ -220,19 +220,21 @@ function topologyActionJson(id, encodedId, name, status, msgTimeout, debug, samp
jsonData["deactivateStatus"] = (status === "ACTIVE") ? "enabled" : "disabled";
jsonData["rebalanceStatus"] = (status === "ACTIVE" || status === "INACTIVE" ) ? "enabled" : "disabled";
jsonData["killStatus"] = (status !== "KILLED") ? "enabled" : "disabled";
- jsonData["startDebugStatus"] = (status === "ACTIVE" && !debug) ? "enabled" : "disabled";
+ jsonData["startDebugStatus"] = (status === "ACTIVE" && loggersTotal!=null && loggersTotal!=0 && !debug) ? "enabled" : "disabled";
--- End diff --
Wouldn't it disable the debug option even when topology.eventlogger.executors is set to null?
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200670850
@arunmahadevan @ptgoetz we are not worried about 0.4 to 0.5% affect on throughput. For most cases no one going to notice that. Lets wait for @roshannaik topology and you can run it and see if its still 0.4% than we can ignore this.
@ptgoetz can you add details how did you run your benchmark.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on a diff in the pull request:
https://github.com/apache/storm/pull/1217#discussion_r58601713
--- Diff: storm-core/src/ui/public/js/script.js ---
@@ -220,19 +220,21 @@ function topologyActionJson(id, encodedId, name, status, msgTimeout, debug, samp
jsonData["deactivateStatus"] = (status === "ACTIVE") ? "enabled" : "disabled";
jsonData["rebalanceStatus"] = (status === "ACTIVE" || status === "INACTIVE" ) ? "enabled" : "disabled";
jsonData["killStatus"] = (status !== "KILLED") ? "enabled" : "disabled";
- jsonData["startDebugStatus"] = (status === "ACTIVE" && !debug) ? "enabled" : "disabled";
+ jsonData["startDebugStatus"] = (status === "ACTIVE" && loggersTotal!=null && loggersTotal!=0 && !debug) ? "enabled" : "disabled";
--- End diff --
Since the intent is to disable event logging by default, we should disable logging if topology.eventlogger.executors is null ... otherwise its confusing.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-197211128
@roshannaik
You may want to elaborate regarding performance degrade, especially how you come up with this patch.
Please feel free to post on dev mailing list, or add comment to JIRA issue, or this pull request.
---
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-1632 Disable event logging by default
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-206454193
@roshannaik can you also disable the events link in component-summary-template if loggers is disabled. It is clickable and points to blank page. If you want, I can put up a fix for this.
---
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-1632 Disable event logging by default
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201133053
@roshannaik I updated the JIRA and removed the fixVersion, Epic for 1.0 release. Lets keep the PR open and see if there is a better option to keep the defaults and not have the perf impact.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200543287
@arunmahadevan :-) ... i am not taking the throughput measurements while profiler is attached.
It will take some time for me to continue to iterate over and analyze your attempts and JProfiler usage to see what is going on. With a quick glance I see multiple differences in your topology setup. But the profiler screenshots that i have posted are hopefully evidence that I didn't cook it up :-). You can either try with the topology i described .. also i shall post a Github link to the topology i am using soon.
@HeartSaVioR I am a bit puzzled to see a 8% or 25% diff in perf (for a given topology) being referred to as *micro* optimization. This is a case of potentially significant overhead being imposed upon the common code path by a infrequently used code path. Quite the contrary, i feel, one should have to have a very good justification to leave this turned on.
It is not feasible to do a full fledged Yahoo style benchmark to identify and fix all such issues. Micro-benchmarking is essential. Here we are looking at a simple case of emit() call dominating most of the time within nextTuple() ... the spout computation itself is taking negligible % of the time.
I have deliberately separated out #1242 from this .. as this is PR about simply disabling a DEBUG config setting.. as opposed to modifying code to avoid repetitive lookups. Seeking and testing an alternative implementation for event logging (unless its trivial) i felt might be tricky at this late stage of 1.x.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201709299
I guess @arunmahadevan removes not only instantiation of Long but also Random.randInt() which makes sense to me.
@roshannaik @arunmahadevan
Btw, please change your BasicTopology to here,
- Roshan's: https://gist.github.com/HeartSaVioR/9fd277307d4d5efcd47f
- Arun's: https://gist.github.com/HeartSaVioR/0e2555633a5f7d12cb68
I pasted functionality about printing metrics from cluster information periodically. (It came from FastWordCountTopology.)
Why this change is necessary is because I saw the behavior that throughput of topology from Roshan isn't be consistent even after 10 mins (it's increasing), so when exactly you refreshed UI can change your numbers. (Please note that 10m is changed, too)
You may want to increase your benchmark period long enough (by modifying loop count in main) to see when its speed becomes stabilized. You may also want to make period as argument.
---
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-1632 Disable event logging by default
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-200261427
@arunmahadevan @roshannaik
I guess I didn't express my thought well, modified last comment.
Anyway, I'm afraid micro optimization with micro benchmark could result in premature optimization easily (but awesome works STORM-1526 and STORM-1539!), and more important thing is that Storm still have design spots to improve performance as you know.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201612733
@arunmahadevan :-) the Java mem mgmt and GC do not work like that... it wont be "eating up 3.2 GB"
---
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-1632 Disable event logging by default
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-201009206
If the detriment does turn out to be fairly small, I would like to see this left on by default. People, I think, are going to be much happier with a feature-full UI by default, and left to disable stuff for super-fast-hyper-performance.
---
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-1632 Disable event logging by default
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the pull request:
https://github.com/apache/storm/pull/1217#issuecomment-203685974
@ptgoetz I have made the changes to the UI as discussed.
Took lot more effort than I imagined. Turns out that adding tooltips on disabled buttons (via Bootstrap that the UI uses for tooltips) requires *advanced* UI skills. Fortunately found a simpler workaround after much experimentation.
---
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.
---