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

Add changes that support settings files #5332

Closed
wants to merge 2 commits into from

Conversation

dedemorton
Copy link
Contributor

Adds doc about settings file to resolve an item listed in #5148

Summary of changes:

  • Formatting improvement requested by @acchen97
  • Add mention of the settings file as an option instead of specifying flags
  • Fix a few minor mistakes and typos
  • Add "coming" annotation to clarify that the changes are for alpha3
  • Added node.name and pipeline.output.workers because it was missing from the doc (I'll add a note below about pipeline.output.workers)

If you find that events are backing up, or that the CPU is not saturated, consider increasing
this number to better utilize machine processing power.

*`--pipeline.output.workers COUNT`*::
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsvd did you mean workers in output plugins?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch @dedemorton

Copy link
Member

Choose a reason for hiding this comment

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

@andrewvc should we expose pipeline.output.workers to the command line and make it known?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, we should not. Its just inconsistent to have 1 config option being exposed this way. We should not set this precedent.

@suyograo
Copy link
Contributor

@dedemorton Can we also add a separate section "Using the Settings File" or something similar to call out the existence of config/logstash.yml in detail? We can then reference this from the Command Line section: https://github.com/elastic/logstash/pull/5332/files#diff-ab5ecd26344a9aea61a0a0581e8f283cR6

Since this is new its good to push its visibility.

-w, --pipeline.workers COUNT
Sets the number of pipeline workers (threads) to run for filter processing (default: number of cores).
*`-w, --pipeline.workers COUNT`*::
Sets the number of pipeline workers (threads) to run for filter processing (default: half the number of cores).
Copy link
Member

@jsvd jsvd May 20, 2016

Choose a reason for hiding this comment

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

I mislead you here, @dedemorton, it's actually the number of cores, not half (set in here)
[edit] please fix also in config/logstash.yml

@dedemorton
Copy link
Contributor Author

@suyograo Sure, I can add the info to a separate topic. I decided to err on the side of minimalism here, but I can see how having a separate topic might raise visibility that the file exists.

@dedemorton dedemorton force-pushed the logstash_issue#4401 branch from aa22440 to 519eee5 Compare May 20, 2016 21:31
@dedemorton dedemorton force-pushed the logstash_issue#4401 branch from 519eee5 to 02342c2 Compare May 20, 2016 21:33
@jsvd
Copy link
Member

jsvd commented May 20, 2016

LGTM

1 similar comment
@suyograo
Copy link
Contributor

LGTM

@elasticsearch-bot
Copy link

DeDe Morton merged this into the following branches!

Branch Commits
master 25fc802, 27882b1
5.0 cc0a7ec, c5fe902

elasticsearch-bot pushed a commit that referenced this pull request May 23, 2016
elasticsearch-bot pushed a commit that referenced this pull request May 23, 2016
elasticsearch-bot pushed a commit that referenced this pull request May 23, 2016
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.

4 participants