Scale http output squash #116

Merged
merged 4 commits into from Oct 8, 2014

Conversation

Projects
None yet
2 participants
@joekiller
Contributor

joekiller commented Sep 24, 2014

This adds dynamic http output worker scaling. It just scales as needed for now. I figure we can add limiters a little later.

README.md
@@ -182,6 +182,8 @@ https://github.com/buger/gor/releases
gor --input-raw :80 --output-tcp replay.local:28020
-split-output=false: By default each output gets same traffic. If set to `true` it splits traffic equally among all outputs.
+
+ -stats=false: If set to `true` it gives out queuing stats for the HTTP output and TCP listener every 5 seconds in the form latest,mean,max,count,count/second.

This comment has been minimized.

@buger

buger Sep 24, 2014

Owner

if it works only for http output make sense to name it: -output-http-stats

@buger

buger Sep 24, 2014

Owner

if it works only for http output make sense to name it: -output-http-stats

This comment has been minimized.

@joekiller

joekiller Sep 24, 2014

Contributor

Yeah we may want it to be -output-http-stats and -output-tcp-stats as that is where it is triggered.

I'll make a separate PR for that if that is okay.

@joekiller

joekiller Sep 24, 2014

Contributor

Yeah we may want it to be -output-http-stats and -output-tcp-stats as that is where it is triggered.

I'll make a separate PR for that if that is okay.

This comment has been minimized.

@buger

buger Sep 25, 2014

Owner

sure

This comment has been minimized.

@buger

buger Sep 25, 2014

Owner

Actually it will be SUPER helpful if you add description of how to read stats output. I remember that you already wrote about it in one of issue discussions. I mean just to add README section on how to read it, what to look for. What worst/good cases.

@buger

buger Sep 25, 2014

Owner

Actually it will be SUPER helpful if you add description of how to read stats output. I remember that you already wrote about it in one of issue discussions. I mean just to add README section on how to read it, what to look for. What worst/good cases.

@buger

This comment has been minimized.

Show comment
Hide comment
@buger

buger Sep 24, 2014

Owner

Awesome work! Will test it soon.

Owner

buger commented Sep 24, 2014

Awesome work! Will test it soon.

output_http.go
o.bufStats.Write(len(o.buf))
-
+ if buf_len > 20 {

This comment has been minimized.

@buger

buger Sep 25, 2014

Owner

Why 20? What his number means?

I mean why it starting to spawn workers only if buf_len > 20, and not lets say 10 ?

@buger

buger Sep 25, 2014

Owner

Why 20? What his number means?

I mean why it starting to spawn workers only if buf_len > 20, and not lets say 10 ?

This comment has been minimized.

@joekiller

joekiller Sep 25, 2014

Contributor

So the incoming buffer holds a maximum of 100 requests and when the requests are written to the buffer it also checks to see how many outstanding requests are sitting on the buffer. If there are more than 20 requests on the queue the length of the queue (n) is passed to the master worker and then the master worker spawns n more http output threads.

I chose 20 via some testing where I found that if I wanted to spawn a worker for each outstanding request I didn't want something too big (ie >50) because too many threads came along and the buffer could be approaching saturation more quickly so I settled on spawning workers when the queue was ~20 behind. 10 would probably work as well.

@joekiller

joekiller Sep 25, 2014

Contributor

So the incoming buffer holds a maximum of 100 requests and when the requests are written to the buffer it also checks to see how many outstanding requests are sitting on the buffer. If there are more than 20 requests on the queue the length of the queue (n) is passed to the master worker and then the master worker spawns n more http output threads.

I chose 20 via some testing where I found that if I wanted to spawn a worker for each outstanding request I didn't want something too big (ie >50) because too many threads came along and the buffer could be approaching saturation more quickly so I settled on spawning workers when the queue was ~20 behind. 10 would probably work as well.

output_http.go
@@ -38,13 +39,13 @@ func ParseRequest(data []byte) (request *http.Request, err error) {
type HTTPOutput struct {
address string
limit int
+ buf chan []byte
+ need_worker chan int

This comment has been minimized.

@buger

buger Sep 25, 2014

Owner

To be consistent and by following go guidelines its better to name it needWorker.
Same with worker_master and worker methods: WorkerMaster and Worker
[codestyle]

@buger

buger Sep 25, 2014

Owner

To be consistent and by following go guidelines its better to name it needWorker.
Same with worker_master and worker methods: WorkerMaster and Worker
[codestyle]

@joekiller

This comment has been minimized.

Show comment
Hide comment
@joekiller

joekiller Sep 25, 2014

Contributor

Thanks for all the comments. I'll try to address the suggestions.

Contributor

joekiller commented Sep 25, 2014

Thanks for all the comments. I'll try to address the suggestions.

Dynamic workers toggle. Separate stats toggles. README and FAQs
  * Created separate toggles for output-http-stats and output-tcp-stats
  * Created HTTPWorkers option (defaults to 10) to allow larger number of HTTPWorkers to be spawned.
  * Refactored output_http to allow dynamic http worker scaling when httpworkers equals -1
  * Updated README.me to include updates and some common FAQs.
@joekiller

This comment has been minimized.

Show comment
Hide comment
@joekiller

joekiller Oct 5, 2014

Contributor

@buger check this PR out now. Tried to incorporate many suggestions.

Contributor

joekiller commented Oct 5, 2014

@buger check this PR out now. Tried to incorporate many suggestions.

@buger

This comment has been minimized.

Show comment
Hide comment
@buger

buger Oct 5, 2014

Owner

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?

Owner

buger commented Oct 5, 2014

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?

@joekiller

This comment has been minimized.

Show comment
Hide comment
@joekiller

joekiller Oct 5, 2014

Contributor

Sure I'll update it a little later.
On Oct 5, 2014 4:53 PM, "Leonid Bugaev" notifications@github.com wrote:

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?


Reply to this email directly or view it on GitHub
#116 (comment).

Contributor

joekiller commented Oct 5, 2014

Sure I'll update it a little later.
On Oct 5, 2014 4:53 PM, "Leonid Bugaev" notifications@github.com wrote:

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?


Reply to this email directly or view it on GitHub
#116 (comment).

@joekiller

This comment has been minimized.

Show comment
Hide comment
@joekiller

joekiller Oct 6, 2014

Contributor

Defaults to dynamic workers now.
On Oct 5, 2014 5:55 PM, "Joseph Lawson" joe@joekiller.com wrote:

Sure I'll update it a little later.
On Oct 5, 2014 4:53 PM, "Leonid Bugaev" notifications@github.com wrote:

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?


Reply to this email directly or view it on GitHub
#116 (comment).

Contributor

joekiller commented Oct 6, 2014

Defaults to dynamic workers now.
On Oct 5, 2014 5:55 PM, "Joseph Lawson" joe@joekiller.com wrote:

Sure I'll update it a little later.
On Oct 5, 2014 4:53 PM, "Leonid Bugaev" notifications@github.com wrote:

This looks really great. Thanks for docs!

Don't you think it make sense to start with dynamic workers by default?


Reply to this email directly or view it on GitHub
#116 (comment).

@joekiller

This comment has been minimized.

Show comment
Hide comment
@joekiller

joekiller Oct 8, 2014

Contributor

@buger how about this? Maybe get a 0.9 release? :-)

Contributor

joekiller commented Oct 8, 2014

@buger how about this? Maybe get a 0.9 release? :-)

@buger

This comment has been minimized.

Show comment
Hide comment
@buger

buger Oct 8, 2014

Owner

Yes, make sense. Will try to do it soon :)

Thank you!

Owner

buger commented Oct 8, 2014

Yes, make sense. Will try to do it soon :)

Thank you!

buger added a commit that referenced this pull request Oct 8, 2014

@buger buger merged commit cf0dfec into buger:master Oct 8, 2014

1 of 2 checks passed

ci/circleci Your tests failed
Details
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment