[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[abhishekagarwal87]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[abhishekagarwal87]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[asfgit]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[knusbaum]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[knusbaum]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

  

```
2 . The tooltip is shown even if the button is enabled. You could add the 
` ` 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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.

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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=1

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=1

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[ptgoetz]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[harshach]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

Github user arunmahadevan commented on the pull request:

https://github.com/apache/storm/pull/1217#issuecomment-20024
  
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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.:


![before](https://cloud.githubusercontent.com/assets/2366541/13939964/d33a5e12-ef96-11e5-89d2-07ff7711cb6e.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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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. 


![profile-3-3](https://cloud.githubusercontent.com/assets/6792890/13916942/c480e89a-ef83-11e5-97f3-e4c0978dd8c5.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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[abhishekagarwal87]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[abhishekagarwal87]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[arunmahadevan]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[HeartSaVioR]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[abhishekagarwal87]

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

[GitHub] storm pull request: STORM-1632 Disable event logging by default

[roshannaik]

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