Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability to specifiy filter worker threads #25

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added ability to specifiy filter worker threads #25

wants to merge 6 commits into from

Conversation

daph
Copy link

@daph daph commented Oct 16, 2015

This change fixes issue #23

It adds the ability to (optionally) specify the amount of worker threads for each test in a suite, by adding :workers => N to the test. If it's not there, it defaults to 1 worker, which is what logstash does by default itself if you don't specify -w to it.

@purbon
Copy link
Contributor

purbon commented Oct 16, 2015

Thanks a lot for your contribution @daph, I'm going to review this very soon.

@purbon
Copy link
Contributor

purbon commented Oct 20, 2015

reviewing

#]
#
[
{:workers => 2, :name => "simple line in/out", :config => "config/simple.conf", :input => "input/simple_10.txt", :time => 120},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why having this example all using 2 workers? what is the reasoning behind it for you? thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need special examples, with workers, I Think we can provide this info by only having one example and good comments. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no real reason why the examples all use 2 workers. It's just more than one and most machines can easily handle 2 threads so I thought it was a decent example. I'm not opposed to changing it up though.

Yeah, you're right, we probably don't need special examples. Maybe just removing these with_workers examples all together and just changing a couple of the tests in the original example files to use workers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your way of thinking about the 2 workers, we should also add documentation for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the with_workers examples and mix the workers config with the former examples. So we end up with some with workers option and some without.

@purbon
Copy link
Contributor

purbon commented Oct 21, 2015

@daph only small comments made, this looks great! as soon as your changes are done I'm happy to merge this! thanks a lot for your time.

@daph
Copy link
Author

daph commented Oct 21, 2015

@purbon I've made changes based on your comments. Hopefully they are to your satisfaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants