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

[Docs] Remove LS_HEAP_SIZE and reference JVM.options instead #9335

Closed
wants to merge 3 commits into from

Conversation

karenzone
Copy link
Contributor

Remove doc references to LS_HEAP_SIZE setting and point to jvm.options file instead. (Fix for #6970.)

Change heading for Setting File to logstash.yml. (I did not change the anchor name.)
The reference guide has a section called Logstash Configuration Files with a subsection called Settings Files. Logstash.yml is documented as an element of Settings Files and is also the subject of a section called Setting File. (This is an artifact from days when logstash.yml was THE setting file. This is my attempt to fix it.)

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM. And I appreciate some of the rewording as well :)

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

Just a couple of minor changes. Otherwise, LGTM!

@@ -100,4 +100,3 @@ Examining the in-depth GC statistics with a tool similar to the excellent https:

NOTE: As long as the GC pattern is acceptable, heap sizes that occasionally increase to the maximum are acceptable. Such heap size spikes happen in response to a burst of large events passing through the pipeline. In general practice, maintain a gap between the used amount of heap memory and the maximum.
This document is not a comprehensive guide to JVM GC tuning. Read the official http://www.oracle.com/webfolder/technetwork/tutorials/obe/java/gc01/index.html[Oracle guide] for more information on the topic. We also recommend reading http://www.semicomplete.com/blog/geekery/debugging-java-performance.html[Debugging Java Performance].
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you've inadvertently trimmed the extra line from the end of the file. Because of how asciidoc processes files, you should include an empty line at the end of each asciidoc file.

@@ -1,5 +1,5 @@
[[logstash-settings-file]]
=== Settings File
=== Logstash.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid capitalizing this filename because it's logstash.yml on disk. I feel that actual names (like filenames and function names) should not be capitalized in titles.

@@ -175,4 +175,3 @@ With this command, Logstash concatenates three config files, `/tmp/one`, `/tmp/t

*`-h, --help`*::
Print help
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above. Do not trim trailing lines. It'll work most of the time, but if somebody adds two includes without a line break between them, your topic might blow up.

@colinsurprenant
Copy link
Contributor

Good stuff! @karenzone In addition to @dedemorton comments about trailing lines, I would also suggest you configure your editor to not remove trailing spaces on lines. It's not a huge deal but as with this PR it creates diffs that are not really actual changes and makes the reviewing process more difficult. I suggest that such cosmetic re-formatting is done as a separate PR.

@elasticsearch-bot
Copy link

karen.metts merged this into the following branches!

Branch Commits
master f5cf0cd, 417b34c, 8ede70d
6.x 4731cb0, fbc2d3f, 77fe203
6.2 92e50d1, 3ac3544, 2be21c5

elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
elasticsearch-bot pushed a commit that referenced this pull request Apr 9, 2018
@dedemorton dedemorton mentioned this pull request Apr 17, 2018
49 tasks
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.

5 participants