subject:"\[GitHub\] storm issue #2241\: STORM\-2306 \: Messaging subsystem redesign."

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
Just leaving a note to make my requirements clear (it is quite simple): 

- new system doesn't break anything it worked
  - if they're unavoidable it should be discussed from Storm community, in 
worst case we decide to disallow to break 
- we should provide default values for relevant variables which makes most 
of topologies happy
  - for this patch it should show higher throughput and lower latency 
compared to default of master branch
- (optionally) we may want to provide specific value for them which makes 
benchmark topologies (or user topologies which runs full speed all the time) 
happier

We also may want to guide how parameters work, and how to tune them, and 
starting values for several use cases so that users can tweak their own 
topologies and find good values for them. I guess we didn't do that before, but 
maybe great to have.

Please let me know when this patch achieves my requirements. I'd rather 
treat this as WIP and just wait for 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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
Btw, I think talking about current state is less meaningful. This patch has 
lots of TODO and some critical identified issues, so it should be addressed, 
and after that the number is going to be really different. We may argue same 
things again and again, so maybe better to hold before this patch becomes 
really ready to review (not WIP).


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
I don't think utilizing metrics consumer in TVL is the issue: it might 
matter if results are close so that contributions of other system component 
does matter, but it is just not acceptable latency for low rate. Huge gap 
between twos.

Let's say we get rid of metrics consumer and that makes stable, then are we 
going to pursue users to not use metrics consumer? That doesn't make sense. 
While I don't think so, but if we think metrics consumer contributes throughput 
and/or latency in really odd way, it needs to be validated and fixed.

As you can see my result, CPU was over 100% even with rate 500 and total 
tasks of three key components were 12 (not 48, please keep in mind). All 
results for TVL was captured in that way. So this patch shows high CPU usage in 
baseline (say minimal load) and shows fluctuation by 80% all over rate 1, 
whereas master branch was 20%.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-27 Thread roshannaik

Github user roshannaik commented on the issue:

https://github.com/apache/storm/pull/2241
  
@revans2 

About that  "better than sliced bread" : 
how could i not be offended.. at least briefly  ;-) but you could buy me 
lunch if this PR turns out better than you were initially afraid of. It  was 
perhaps a very "low latency" -1 for any PR in the history of Storm. :-) 

Yes there are rough edges and some bugs... but I do dream of being able 
nail it all the way in one go. 

Your observation about the **very high latency for low throughput** topos. 
That is clearly a problem with batch not filling up and not getting flushed. 
The 5 sec latency corresponds to the 'topology.flush.tuple.freq.millis' setting 
(default 5sec). So at each step between Spout->Bolt and Bolt->Bolt if the its 
waiting for 5 sec then you are likely to see such ridiculous latency numbers.

Given that, I think the solution must be evident by now ...  but will state 
it here for the benefit of other readers to whom it may not be:

Tweak one or both of  these two settings:
- **topology.producer.batch.size** : for low throughput topos setting this 
to 1 is a good idea. In the new system the throughput penalty is not that for a 
small batch much compared to larger batch size.  
- **topology.flush.tuple.freq.millis**: You could reduce this for more 
frequent batch flushing. It causes the timer thread to take up more cpu... but 
if throughput fluctuates between very and very low over the day, then is 
setting maybe better to meet latency SLA.



---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@revans2 I am trying to reproduce the worst-case in your last chart. 
Running TVL topology with 4 spout, 10 splitters, 4 counters, 2 ackers.  Here is 
the code
https://gist.github.com/harshach/73dae347c178ac5dd8651cb0e7902412
Running it via following command against Master and STORM-2306 
`/bin/storm jar /tmp/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency 500 1 -c topology.workers=1 -c 
topology.max.spout.pending=500 -c topology.acker.executors=2`

You can look at my results here 
https://docs.google.com/spreadsheets/d/1wPpC3YXp-vTIelRTUVoLxuxIYxUekiIHUpZ2ZEysC4Y/edit#gid=1239810430
 in **sheet 2**
What I see not much difference between Master and STORM-2306. Let me know 
if I am missing something in running this test.



---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@revans2 I am trying to reproduce the worst-case in your last chart. 
Running TVL topology with 4 spout, 10 splitters, 4 counters, 2 ackers.  Here is 
the code
https://gist.github.com/harshach/73dae347c178ac5dd8651cb0e7902412
Running it via following command against Master and STORM-2306 
`/bin/storm jar /tmp/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency 500 1 -c topology.workers=1 -c 
topology.max.spout.pending=500 -c topology.acker.executors=2`

You can look at my results here 
https://docs.google.com/spreadsheets/d/1wPpC3YXp-vTIelRTUVoLxuxIYxUekiIHUpZ2ZEysC4Y/edit#gid=1239810430
 in **sheet 2**
What I see not much difference between Master and STORM-2306. Let me know 
if I am missing something in running this test.



---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-27 Thread roshannaik

Github user roshannaik commented on the issue:

https://github.com/apache/storm/pull/2241
  
Some points covering prev comments by @HeartSaVioR and @revans2 

**Throughput limiting:** That only makes sense if you are measuring 
Throughput vs CPU/other resource usage.  Latency measurements do not need it. 
And its a sin if you are doing that when trying to measure throughput.

**TVL topology:** 
- Given its rate limiting nature, it definitely does not have the right 
name. Its employment of very high threads counts and rate limiting spouts 
appear to be tuned to work within the limitations of the current msging system 
and target the old sweetspot. Deserves a question.  Harsha's measurements 
(which are more sensible in terms of executor counts), shows that the current 
msging was brought down to its knees very quickly once the rate limiting went 
away.  


@revans2 
The drop you are seeing with the increased in splitter counts is indicative 
of the increased CPU contention going on even when not enough data flowing 
through an executor (the issue you initially brought up... of high CPU usage 
for idle topos).  The old system, executor seems to be spending more time 
sleeping when there is insufficient data flow and less CPU contention and 
adding redundant/idle executors is not affecting it as much.So you can 
throughput plateaus. 

Lowering the CPU contention for idle mode is something i plan to address... 
and i think have left some TODOs for myself in the code already for to keep me 
honest.



---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-27 Thread roshannaik

Github user roshannaik commented on the issue:

https://github.com/apache/storm/pull/2241
  
For the new messaging system.. the scaling rule of thumb I have found so 
far is quite simple. 

For fast topos (and CPU intensive topos) ... 1 executor thread per 
*physical core*.  It applies to ACKer executors as well. Avoid trying to max 
out on logical cores / hyperthreads.

You are likely to be close to getting the most out of your hardware with 
that rule. You can start with that and try adding/removing one or more 
executors to see if you can squeeze more.  

The older system will typically need more executors per machine to get 
similar numbers (throughput usage).. but  throughput may not come close to 
the new system.

The rule for executors count v/s CPU cores for the existing msgs system 
seems less simple to me.

Trying to run 51 executors on a 4 core machine will surely be a step 
towards "de-scaling", if there is such a word. It is strange that such high 
executor count was useful in the current system.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
I have another chart now showing a comparison between master and this 
branch, just varying the number of splitter bolts in the topology.  There are 2 
ackers, 4 spouts, and 4 count bolts all within a single worker and with a max 
spout pending set to 500.  All of the configs are the defaults, and it is on my 
laptop like before.


![chart](https://user-images.githubusercontent.com/3441321/28689893-de51856c-72dc-11e7-8f5b-8f2a77efdfd3.png)

The scary thing here is that with these changes there is a tiny window 
where you get "good" throughput (lets say above 150k sentences per second) for 
this branch.  The previous branch has a very very wide window.  The thing that 
concerns me the most with the way it is now, is that there will be a lot of 
people who didn't turn the parallelism as low as possible, because it just 
works, and they will all have their topologies go from 180k/sec down to 
50k/sec.  And tuning them all perfectly to balance on that exact parallelism 
for the given heterogeneous hardware that we run on is going to be impossible.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
I have run some more tests looking at modifying the parallelism of 
different components.

First I kept the parallelism of everything else at 4 and modified the acker 
count.

![chart_ackers](https://user-images.githubusercontent.com/3441321/28684646-bc460734-72ca-11e7-9434-8bdf2c263cab.png)

I also kept the ackers at 2 spout and count at 4 and modified the splitter 
count

![chart_splitters](https://user-images.githubusercontent.com/3441321/28684647-bc462f5c-72ca-11e7-91f8-0a4e1c748682.png)

The acker drop off at 5 is really scary,  but adding too many splitters 
also shows a lot of problems.  I am going to try something similar without the 
patch for comparison.

Overall the numbers look really good in some situations, but it is really 
easy to slip into much worse territory.  @knusbaum said that he was able to get 
a multi-worker setup to work, so that is something else I want to explore. 



---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
Sorry, but we also need to think about low throughput use cases.  I have 
several that I care about and I am seeing very long latency for low throughput. 
 

min in some cases is 5 seconds, max can be up to 20 seconds, average is 
around 10 seconds and the CPU utilization is 500%.  This too needs to be 
addressed.

```
500 1 -c topology.workers=1
uptime:   30 acked: 4,000 acked/sec: 133.33 failed:0 99%:   
9,923,723,263 99.9%:   9,999,220,735 min:  79,036,416 max:  10,015,997,951 
mean: 5,861,829,371.65 stddev: 2,744,502,279.38 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:   60 acked:15,000 acked/sec: 500.00 failed:0 99%:  
14,646,509,567 99.9%:  14,973,665,279 min:  53,084,160 max:  15,023,996,927 
mean: 7,410,713,531.31 stddev: 3,187,842,885.35 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:   90 acked:16,000 acked/sec: 533.33 failed:0 99%:  
14,747,172,863 99.9%:  14,990,442,495 min:  37,486,592 max:  15,032,385,535 
mean: 7,947,532,282.45 stddev: 3,104,232,967.22 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  120 acked:14,000 acked/sec: 466.67 failed:0 99%:  
14,856,224,767 99.9%:  14,998,831,103 min:  65,208,320 max:  15,023,996,927 
mean: 9,071,752,875.48 stddev: 3,337,053,852.19 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  150 acked:13,000 acked/sec: 433.33 failed:0 99%:  
14,914,945,023 99.9%:  14,998,831,103 min:   4,999,610,368 max:  15,074,328,575 
mean: 10,374,946,814.88 stddev: 2,794,778,136.42 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  180 acked:16,000 acked/sec: 533.33 failed:0 99%:  
14,940,110,847 99.9%:  15,049,162,751 min:   5,007,998,976 max:  15,602,810,879 
mean: 10,539,964,609.74 stddev: 2,796,155,497.39 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  210 acked:15,000 acked/sec: 500.00 failed:0 99%:  
14,881,390,591 99.9%:  14,998,831,103 min:   5,003,804,672 max:  15,015,608,319 
mean: 9,616,077,147.72 stddev: 2,781,415,317.06 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  240 acked:10,000 acked/sec: 333.33 failed:0 99%:  
14,889,779,199 99.9%:  15,007,219,711 min:   5,003,804,672 max:  15,015,608,319 
mean: 9,840,073,724.86 stddev: 2,806,028,726.32 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  270 acked:16,000 acked/sec: 533.33 failed:0 99%:  
17,951,621,119 99.9%:  19,780,337,663 min:   5,003,804,672 max:  20,015,218,687 
mean: 10,556,609,171.18 stddev: 3,010,780,308.43 user:  0 sys:  
0 gc:  0 mem:   0.00
uptime:  300 acked:15,000 acked/sec: 500.00 failed:0 99%:  
14,898,167,807 99.9%:  14,998,831,103 min:  51,445,760 max:  15,023,996,927 
mean: 9,694,508,448.06 stddev: 3,087,190,409.09 user:  0 sys:  
0 gc:  0 mem:   0.00
```

I am fine with the goals and the design work being done for this.   If you 
can do better then the stuff I did for disruptor by all means rip out my code 
and make things better.  The low throughput issue was one I had to fix with my 
initial patches to disruptor.  People do care about this.  I am not trying to 
be a jerk, I am just trying to keep my customers happy, share some of my 
experience doing something similar in the past, and also hopefully make storm 
much much better in the end.

I apologize if  I offended anyone.  It was not my intention, but I really 
was shocked to see a patch everyone was touting as better than sliced bread 
decidedly worse in every way for a topology that worked really well before.  I 
was able to max out the default configuration of a parallelism of 4 at 100,000 
sentences per second fully acked.  The new patch could only handle 1/3rd of 
that, and not when there is more then 1 worker.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
@harshach 

Reiterating what @HeartSaVioR said about benchmarking.  Most benchmarking 
is done where you push a system to its limits and see what maximum throughput 
it can do.  This is far from what a real user wants.  It looks good for a 
vendor to brag about I can do X but that other vendor over there can only do Y. 
 But it is close to worthless for what real users want to know.

Real users are trying to balance the cost of the system in $ (CPU time + 
memory used become this, how many EC whatever instances do I need), the amount 
of data that they can push through the system and how quickly they can get 
results back.  Each of these variables are reflected by this test.  In most 
cases a user has a set load that they know they get typically, and a reasonable 
guess at a maximum load that they expect to see.  Also most users have a 
deadline by which the data is no good any more, if not they should be using 
batch.  And a budget that they have to spend on this project, if not call me I 
want to work for you and my salary requirements are very reasonable.

You need to give users tools to explore all three, and because the three 
are intertwined you want to be able to hold one or two of the variables 
constant while you measure the others.  Storm currently has no way to set a 
target SLA (I hope to add one eventually), but you can control the rate at 
which messages arrive and the parallelism of the topology, (which reflects the 
cost).  So the goal is to scan through various throughput values and various 
parallelisms to see what the latency is, and what resources are actually used.  
In the read world we would adjust the heap size and parallelism accordingly.

Complaining about a benchmark creating 51 threads relates to the 
parallelism that we want to explore.  If that is what I did wrong in the 
benchmark I am happy to adjust and reevaluate.  I want to understand how the 
parallelism impacts this code.  The thing that concerns me now is that it 
appears that scaling a topology is very different now, and I want to understand 
exactly how that works.

I cannot easily roll out a change to my customers saying things might get a 
lot better or they might get a lot worse.  We need to make it easy for a user 
with a topology that may not have been ideal (but worked well), to continue to 
work 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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
I have updated all the results for TVL with second parameter set to 1. Also 
added rate 5.
The CPU usage from current master doesn't fluctuate from all of rates, even 
5, whereas with this patch the CPU usage sometimes fluctuate around 100%.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
Let me share a quick test result with passing `1 1` to TVL parameter:

> STORM-2306

```
uptime:   30 acked:   144,000 acked/sec:   4,800.00 failed:0 99%:   
3,070,230,527 99.9%:   3,221,225,471 min:  63,897,600 max:   3,380,609,023 
mean: 1,299,365,069.36 stddev:  685,287,508.16 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:   60 acked:   303,000 acked/sec:  10,100.00 failed:0 99%:   
3,011,510,271 99.9%:   3,200,253,951 min:  28,540,928 max:   3,303,014,399 
mean: 1,283,728,691.41 stddev:  671,791,145.42 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:   90 acked:   297,000 acked/sec:   9,900.00 failed:0 99%:   
3,047,161,855 99.9%:   3,307,208,703 min:  62,980,096 max:   3,737,124,863 
mean: 1,283,141,447.64 stddev:  675,126,086.16 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  120 acked:   303,000 acked/sec:  10,100.00 failed:0 99%:   
3,047,161,855 99.9%:   3,206,545,407 min:  31,965,184 max:   3,347,054,591 
mean: 1,284,140,763.79 stddev:  690,625,730.54 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  150 acked:   299,000 acked/sec:   9,966.67 failed:0 99%:   
3,072,327,679 99.9%:   3,231,711,231 min:  16,703,488 max:   3,414,163,455 
mean: 1,320,620,493.23 stddev:  693,327,734.87 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  180 acked:   300,000 acked/sec:  10,000.00 failed:0 99%:   
3,042,967,551 99.9%:   3,248,488,447 min:  48,005,120 max:   3,846,176,767 
mean: 1,313,068,274.86 stddev:  671,810,427.83 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  210 acked:   301,000 acked/sec:  10,033.33 failed:0 99%:   
3,061,841,919 99.9%:   3,363,831,807 min:  51,347,456 max:   3,802,136,575 
mean: 1,297,807,219.57 stddev:  678,980,965.35 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  240 acked:   301,000 acked/sec:  10,033.33 failed:0 99%:   
3,019,898,879 99.9%:   3,208,642,559 min:  36,962,304 max:   3,363,831,807 
mean: 1,315,037,518.24 stddev:  676,620,121.79 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  270 acked:   297,000 acked/sec:   9,900.00 failed:0 99%:   
3,026,190,335 99.9%:   3,200,253,951 min:  52,363,264 max:   3,349,151,743 
mean: 1,308,161,023.51 stddev:  680,121,348.29 user:  0 sys:  0 
gc:  0 mem:   0.00
uptime:  300 acked:   300,000 acked/sec:  10,000.00 failed:0 99%:   
3,021,996,031 99.9%:   3,200,253,951 min:  49,348,608 max:   3,317,694,463 
mean: 1,335,928,012.31 stddev:  667,642,145.32 user:  0 sys:  0 
gc:  0 mem:   0.00
```

CPU usage was around 150 ~ 250%, mostly around 160% which seemed to be a 
bit more stable, but still fluctuating with small rate.

> current

```
uptime:   30 acked:   140,440 acked/sec:   4,681.33 failed:0 99%:   
   14,016,511 99.9%:  26,558,463 min:   2,449,408 max:  52,035,583 
mean:7,800,556.68 stddev:1,790,982.79 user: 28,620 sys:  2,340 
gc:  0 mem:  49.94
uptime:   60 acked:   301,860 acked/sec:  10,062.00 failed:0 99%:   
   11,141,119 99.9%:  15,351,807 min:   3,233,792 max:  26,181,631 
mean:7,479,081.72 stddev:1,175,253.40 user: 30,270 sys:  6,800 
gc:190 mem:  54.88
uptime:   90 acked:   301,600 acked/sec:  10,053.33 failed:0 99%:   
   10,813,439 99.9%:  13,197,311 min:   3,246,080 max:  16,138,239 
mean:7,375,841.06 stddev:1,112,541.35 user: 31,660 sys:  7,160 
gc:194 mem:  54.68
uptime:  120 acked:   301,460 acked/sec:  10,048.67 failed:0 99%:   
   11,042,815 99.9%:  13,828,095 min:   3,266,560 max:  17,285,119 
mean:7,400,672.94 stddev:1,130,409.32 user: 29,650 sys:  7,330 
gc:200 mem:  47.80
uptime:  150 acked:   301,500 acked/sec:  10,050.00 failed:0 99%:   
   10,911,743 99.9%:  13,246,463 min:   3,248,128 max:  15,654,911 
mean:7,399,041.82 stddev:1,118,368.85 user: 29,920 sys:  7,370 
gc:199 mem:  41.95
uptime:  180 acked:   301,540 acked/sec:  10,051.33 failed:0 99%:   
   10,969,087 99.9%:  13,598,719 min:   3,233,792 max:  16,302,079 
mean:7,390,435.62 stddev:1,129,976.16 user: 29,840 sys:  7,190 
gc:201 mem:  41.16
uptime:  210 acked:   301,540 acked/sec:  10,051.33 failed:0 99%:   
   11,182,079 99.9%:  14,557,183 min:   3,246,080 max:  19,513,343 
mean:7,382,121.55 stddev:1,161,863.92 user: 29,620 sys:  7,460 
gc:

[GitHub] storm issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
@harshach The second argument is effectively representing worker count: you 
can see that topology set worker count as parallelism. I agree that the name is 
really misleading even I ran tests with topology.workers instead of passing 
second argument. (need to run test again...)


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@HeartSaVioR Its not 12 executors per worker.  If you don't pass a 
command-line argument, it sets parallelism variable here to 4 
https://github.com/apache/storm/blob/master/examples/storm-starter/src/jvm/org/apache/storm/starter/ThroughputVsLatency.java#L277
and multiplys with 4 here again 
https://github.com/apache/storm/blob/master/examples/storm-starter/src/jvm/org/apache/storm/starter/ThroughputVsLatency.java#L359
 . So setting a parallelism unit 16 per component. 
This is nothing to do with how many workers you've.


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

Re: [GitHub] storm issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-27 Thread Alexandre Vermeerbergen

Hello Roshan,

Thank you for your detailed answer. Details are important, because in my
organization, I am often asked to re-assess the reasons why we chose Storm
over its competitors.

Best regards,
Alexandre Vermeerbergen


2017-07-25 23:36 GMT+02:00 roshannaik :

> Github user roshannaik commented on the issue:
>
> https://github.com/apache/storm/pull/2241
>
> @avermeer Looks like SuperChief blog is relaying the same basic claims
> that Heron has marketed. Since you ask, i will share my opinions wrt
> Heron's claims.
>
> - Heron has never been a player in the high performance club. They
> have been smart about not comparing themselves with the real top performers
> of the day. I only included them here because they have built they have
> made much noise against Storm. They are smart about not mentioning which
> version of Storm they are comparing with (how does a paper with such
> critical info missing get accepted ?). That creates an illusion in people
> that their perf claims apply to all versions of Storm in general... even if
> Storm [publishes new perf numbers](hortonworks.com/blog/
> microbenchmarking-storm-1-0-performance/) comparing itself to a prior
> version.
> - Heron's threading model (1 thread per process.. based on what i
> gather from their articles), is really primitive for this application
> domain.  I don't recommend it, but by setting 'topology.workers' equal to
> the number of spout& bolt instances, Storm can be run in Heron mode.
> -  I find it much easier to debug a process with multiple components
> using a debugger rather start a separate debugger for every instance of
> spout bolt running. Also, I would imagine, having so many processes means
> you have an explosion of log files to deal with when triaging.
> - Unclear why the recovery model (when worker process crashes) is any
> better ... the same kind of replay from the spout would be required. The
> gains may be minor if any. Making minor optimizations to the failure path
> and penalizing the normal operation path... is backwards.
> - Cant get a stack from a Storm worker ? Thats clearly false. Try it
> yourself. I do it all the time. Heapdumps, on the other hand, can stall the
> worker and if the heap size is really large the supervisor might feel the
> worker is having a problem. There are timeouts that you can increase to for
> the supervisor to wait longer. I cant imagine that Heron doesn't monitor
> their workers and restart them if they are not responsive.
> -  Heron's Backpressure model is simply too overweight, but marketed
> as a novel idea.
> - A quick read of their latest perf blog, noted in the comparison, and
> it was evident that they missed recognizing their real perf problem.
>
>
>
> ---
> 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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241

@harshach
For ThroughputvsLatency, throttling spout is intended. We set desired
throughput and see histogram of latency and other metrics. (CPU, GC, etc.)
There're 3 components in topology which parallelism would be set to 4 * worker
count so total 12 executor threads per worker. I think we can parameterize the
magic number 4 and adjust it while testing too.

I have also done with some performance tests, without modifying TVL
topology. The reason is that we should also care about
non-performance-maximized topology. For benchmarking performance maximized
topology we also have ConstSpoutIdBoltNullBoltTopo, so let's not modify TVL and
verify this patch works with all the cases.

Since this patch doesn't seem to handle inter-worker communication
properly, the test set what we can do for now is very limited.

Here's my machine spec used for performance test:

```
java version "1.8.0_131"
Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)
```

```
OS: Ubuntu 17.04
CPU: AMD Ryzen 5 1600 3.2Ghz 6 core (with hyper-thread = 12 logical cores)
RAM: Samsung DDR4 32G 19200
SSD: Samsung 850 Evo
```

and here's my number (just pasted as raw number):

https://docs.google.com/spreadsheets/d/1J3S4R68CsazlINF60rQn4XCy2Hx5QNzqVoWSO9qo2tc/edit?usp=sharing

My observation is that this patch looks impressive with performance
maximized topology, but this also looks really bad (not acceptable) with
relatively idle topology. I've observed all the things what @revans2 observed
with TVL tests. But this patch looks stable with ConstSpoutIdBoltNullBoltTopo
and even CPU usage seems lower than stock in this test.

While we often publicize micro-benchmark result, in practice users would
run much idle topologies.
I'm OK if things can be stabilized with adjusting parameters (if then I
think default value should be here), but if not, it should be addressed before
accepting the patch. I would be -1 if TVL result is not stable.

---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241

@revans2 @HeartSaVioR
Here are my findings
https://docs.google.com/spreadsheets/d/1wPpC3YXp-vTIelRTUVoLxuxIYxUekiIHUpZ2ZEysC4Y/edit#gid=1644511...

1. Looking at ThroughputvsLatency I found some issues:
- By default it adds 51 total threads , that IMO is incorrect when
benchmarking in a 4-core machine.

- Also it adds two bolts for logging/measurements which might be impacting
the numbers

https://github.com/apache/storm/blob/master/examples/storm-starter/src/jvm/org/apache/storm/starter/...

- It also throttles the spout

https://github.com/apache/storm/blob/master/examples/storm-starter/src/jvm/org/apache/storm/starter/...

I did the following changes:
- Disable the HTTP and Logging bolts
- Disable throttling spout, we want spout to run as fast as it can
- reduced the executor counts

If you see lines from 78 - 102.

Apache Master clearly couldn't handle the faster spout and starts timing
out. Perf degrades considerably and very quickly. Where as STORM-2306 not only
was able to handle the faster spout and delivered stable and processing at more
start out being 10x faster then improves to 35x faster compared to master.

2. Also ran storm-perf topologies ConstSpoutIdNullBoltIdTopo and
ConstSpoutNullBoltTopo. These topologies are trying to see whats the message
throughput and latency when there are only 2 components involved without
including any external dependencies. Essentially testing the messaging system.

From line 3-45 you can see with this patch we are getting under 10ms
(depends on the topology) compare to an avg of 250ms+. (with batchSize=1)

3. Also ran storm-examples ThroughputVsLatency with 2 workers. Here there
is clearly a bug which is prevent inter-worker communication so don't have
comparative numbers.

[GitHub] storm issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-26 Thread harshach

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@HeartSaVioR I don't mind breaking this into multiple PRs if it helps 
reviewing and merging in. Its up to @roshannaik .


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
@harshach 
As I commented multiple times, this patch is mixed up of replacing queue, 
changing message subsystem, and micro-optimization. Three committers already 
had a look at the first pass (excluding @revans2 since he stopped reviewing) 
but we couldn't identify any critical problems unless @revans2 shared the test 
result. If we go on with this patch, we even don't know which part of change 
contribute s performance gain and how much it is.

If we were having PR only replacing JCTools, it is easy to review and 
identify the benefit, or pros/cons of replacing Disruptor with JCTools. The 
patch will be fairly simple to review and verify, and most of the issues raised 
here shouldn't be raised there (because I guess most of them are not from queue 
replacement).


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-26 Thread harshach

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@HeartSaVioR lets keep this discussion to reviews. This is not forum to 
discuss what one should tweet or not that's up to individuals. Nobody is trying 
to promote something that's not feasible lets not try to be a moral authority 
here to suggest what one can do or not. 
Regarding breaking this into multiple PRs addressing different subsystems, 
that's a reasonable ask. But lets wait before we go down that path we need to 
look into the issues raised here and reproduce the case.
I am running few tests myself and I'll report my findings.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
From now what I can suggest is... breaking down STORM-2306 to "redesign 
messaging subsystem" and "switch to JCTools Queues", and try out latter part 
first.
Let's just have new pull request only replacing Disruptor with JCQueue and 
see it helps. I'd rather lean on putting the minimal effort and make it better 
incrementally. This patch mixes up everything (micro optimization, switching 
queue, messaging architecture change) in one, so hard to point out where the 
issue starts.
And let's separate micro-optimization (converting map usage to array list, 
avoiding iterator, etc.) as another issue and don't address them from other 
issues. I think we should have better throughput even without them if newer 
subsystem is better and/or JCTools Queues are better than Disruptor in our case.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
First of all, I think I should be more careful about like / retweet. Sorry 
about that and just reverted all.

Regarding tweet, I think it heavily depends on which accounts (who) tweet 
it.

If users or contributors are tweeting about this PR, we can take it like a 
gossip and no problem on it.

If committers or PMCs are doing it, that could be shown as kind of 
publicizing, especially this PR compares with other frameworks and claims Storm 
will be no 1. performer. If PMC members tweet about unsafe or not yet stable 
source, and if it turned out to another result, someone could feel we (Storm 
community) hype, regardless of intention.
If we say this PR could increase throughput by N times from before, that 
would be less problematic. I don't say no problem, but much less.

If we are doing it with official account (@ApacheStorm), that just matters, 
because the account could be shown as on behalf of Storm community or at least 
PMC members. I was the one wondering why official account retweeted about 
non-Storm related tweet like 
https://twitter.com/BoredElonMusk/status/889935279213223936 . 
If we haven't talked about which things official account should (not) 
tweet, we may need to have a time to talk about.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
@harshach I am running with defaults in all cases

I build `mvn clean install -DskipTests`
package `cd storm-dist/binary; mvn clean package`
untar the result `tar -xzvf 
./final-package/target/apache-storm-2.0.0-SNAPSHOT.tar.gz`
move the directory so I can save it if I want to switch back and forth `mv 
apache-storm-2.0.0-SNAPSHOT under-test`
then bring up the cluster
```
cd under-test;
./bin/storm dev-zookeeper &
./bin/storm nimbus &
./bin/storm supervisor &
./bin/storm ui &
./bin/storm logviewer &
```

Wait for everything to come up, and I can see the ui.

Then I run some tests (I pull in the storm starter from the build because 
the packaged one does not build on its own).
```
./bin/storm jar 
../../../examples/storm-starter/target/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency
...
./bin/storm jar 
../../../examples/storm-starter/target/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency -c topology.workers=1
...
./bin/storm jar 
../../../examples/storm-starter/target/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency 1 -c topology.workers=1
...
./bin/storm jar 
../../../examples/storm-starter/target/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency 1 -c topology.workers=1 -c 
topology.max.spout.pending=500
...
./bin/storm jar 
../../../examples/storm-starter/target/storm-starter-2.0.0-SNAPSHOT.jar 
org.apache.storm.starter.ThroughputVsLatency 1 -c topology.workers=1 -c 
topology.max.spout.pending=1000
```


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

2017-07-26 Thread harshach

Github user harshach commented on the issue:

https://github.com/apache/storm/pull/2241
  
@revans2 Do you mind posting your storm.yaml or are you running with 
defaults. We will try to see if we can reproduce this same behavior on our 
side. If there are any bugs we will work to fix it and but its shows great 
potential on the perf improvements. 
Regarding posting to twitter , Yes we are very excited about the patch and 
definitely want to share the results with the community. Not sure why you are 
getting upset about it. Its important that we make these perf improvements and 
also let the community know that there are continuous improvements in Storm. If 
you found a bug thats great thats why we've PR and review process in place. 


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
I tried on Linux too and got very similar results.  The CPU and memory 
usage of the topology was lower but the actual throughput and latency of the 
topology was very 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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
I ran again with this exact version 
(5c0db923ecd8e4e1ce0e325ee2fd0f25bae7b0c2) and got the same results.


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user revans2 commented on the issue:

https://github.com/apache/storm/pull/2241
  
-1

Perhaps I am running into some odd issues here so if I can be corrected I 
would be happy to change my vote, but nothing I have run with this patch is 
better in any way.  Are all of the results from micro benchmarks?  Did anyone 
run a real topology with this patch before posting all of these wonderful 
results to twitter?  I am not one to swear but WTF?

I built a stock 2.0.0-SNAPSHOT build 
(450ed637f92c3f387681a47b4b667f17eeecac1f) and compared it to the exact same 
release with this patch merged on top of it (which was a clean merge).  I am 
running

```
$ java -version
java version "1.8.0_121"
Java(TM) SE Runtime Environment (build 1.8.0_121-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.121-b13, mixed mode)

Sierra 10.12.6
MacBook Pro (Retina, 15-inch, Mid 2015)
2.8 GHz Intel Core i7
16 GB 1600 MHz DDR3
```

I ran the ThroughputVsLatency topology with several different options and 
no changes at all to the default storm.yaml.

With this patch I found that.

1. Running a topology with more than one worker appears to not be able to 
send any messages between workers (or it takes so long most of the messages 
time out). So I switched all of my tests to a single worker.
2. When processing a nearly idle topology (500 sentences/second) the CPU 
utilization was everything that my box could give it.  (8 cores fully utilized) 
compared to about one half of one core used by stock storm.
3. The latency is absolutely horrible.  The minimum latency for a somewhat 
idle topology was 1 to 4 seconds to do a word count.  For a topology processing 
10,000 sentences per second it dropped to 800 ms.  The maximum latency was 15 
seconds for all of these cases.  Compare that to stock storm which has a min 
latency of around 3 to 4 ms for the normal case.
4. The system bolt metrics do not work, or at lest I was not able to get 
any of them back.  I tried to compare memory and CPU usage through top, which 
worked out OK.
5. memory usage is insane.  The resident memory was 2 GB for almost all of 
the workers no matter the throughput.  That is 1 GB more than stock storm for 
the same 10,000 sentences per second.
6. The maximum throughput is about 1/4th what it is on stock storm.  I was 
able to to 100,000 sentences per second on my laptop.  I could do it both with 
a single worker, and with 4 worker (although the CPU usage was higher in the 
latter case). With this patch I was able to do 30,000 sentences per second in 
the best case, but on average it could only do about 25,000.

I am happy to make all of my numbers public.  I also plan to run them on a 
Linux box to see if it is any different. 


---
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 issue #2241: STORM-2306 : Messaging subsystem redesign.

Github user HeartSaVioR commented on the issue:

https://github.com/apache/storm/pull/2241
  
Btw, we should be careful if we drop something we provided. This patch 
seems focus to performance optimization (especially micro), and for some spots 
it might not be always better. One example is disabling load aware 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.
---

[GitHub] storm issue #2241: STORM-2306 : Messaging subsystem redesign.