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.

Sign up for GitHub

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

Jump to bottom

[DOCS] Update reference documentation that mentions CMS #50542

Merged
merged 2 commits into from
Jan 7, 2020
Merged

[DOCS] Update reference documentation that mentions CMS #50542

merged 2 commits into from
Jan 7, 2020

Conversation

ebadyano
Copy link
Contributor

Relates to #46973

@ebadyano ebadyano added >docs General docs changes :Core/Infra/Settings Settings infrastructure and APIs labels Dec 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Settings)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

dliappis
dliappis reviewed Jan 2, 2020
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thanks! Left a suggestion for rephrasing in case it makes things clearer.

docs/reference/setup/bootstrap-checks.asciidoc Outdated
@@ -187,7 +187,7 @@ you must not start Elasticsearch with the serial collector (whether it's
from the defaults for the JVM that you're using, or you've explicitly
specified it with `-XX:+UseSerialGC`). Note that the default JVM
configuration that ships with Elasticsearch configures Elasticsearch to
use the CMS collector.
use the CMS collector prior to JDK 14 and to use the G1GC collector with JDK 14+.
Copy link
Contributor

Choose a reason for hiding this comment

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

how about:

use the G1GC collector starting with JDK 14 onwards and defaults to the CMS collection for older JDK versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should reorder this so that G1GC comes first. However, I think we should probably break this into two sentences for better clarity.

use the G1GC garbage collector with JDK14 and later versions. For earlier JDK versions, the configuration defaults to the CMS collector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review @jrodewig and @dliappis. I updated the doc according to your comments.

@dliappis dliappis requested a review from jrodewig January 2, 2020 13:09
jrodewig
jrodewig reviewed Jan 2, 2020
View reviewed changes
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

I left a suggestion that might help with clarity a bit. Please let me know if you have any questions.

jrodewig
jrodewig approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ebadyano.

@ebadyano ebadyano merged commit bb736f7 into elastic:master Jan 7, 2020
@ebadyano
Copy link
Contributor Author

ebadyano commented Jan 7, 2020

Thank you for the review @jrodewig!

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
@ebadyano @SivagurunathanV
[DOCS] Update reference documentation that mentions CMS (elastic#50542)
c241318 
Relates to elastic#46973
@kostasb
Copy link

kostasb commented Dec 2, 2020

@jrodewig This PR has not been backported to 7.x

Our current documentation in https://www.elastic.co/guide/en/elasticsearch/reference/current/_use_serial_collector_check.html still mentions

Note that the default JVM configuration that ships with Elasticsearch configures Elasticsearch to use the CMS collector.

which is incorrect for versions 7.7 or above and needs to be updated with the content of this PR.

Please, could you help address this?

This was referenced Dec 2, 2020
Merged
Merged
jrodewig added a commit that referenced this pull request Dec 2, 2020
@jrodewig @ebadyano
[DOCS] Update reference documentation that mentions CMS (#50542) (#65733
ea7d8d1 
)

Relates to #46973

Co-authored-by: Evgenia Badyanova <[email protected]>
jrodewig added a commit that referenced this pull request Dec 2, 2020
@jrodewig @ebadyano
[DOCS] Update reference documentation that mentions CMS (#50542) (#65734
b829176 
)

Relates to #46973

Co-authored-by: Evgenia Badyanova <[email protected]>
jrodewig pushed a commit that referenced this pull request Dec 2, 2020
@ebadyano @jrodewig
[DOCS] Update reference documentation that mentions CMS (#50542)
0df3a49 
Relates to #46973
jrodewig pushed a commit that referenced this pull request Dec 2, 2020
@ebadyano @jrodewig
[DOCS] Update reference documentation that mentions CMS (#50542)
f450b7a 
Relates to #46973
jrodewig pushed a commit that referenced this pull request Dec 2, 2020
@ebadyano @jrodewig
[DOCS] Update reference documentation that mentions CMS (#50542)
6c833f1 
Relates to #46973
@jrodewig
Copy link
Contributor

jrodewig commented Dec 2, 2020

Thanks @kostasb. I've backported these changes.

@tsouza
Copy link

tsouza commented Dec 2, 2020

Cloud documentation also refers to CMS. It also mentions the 75% threshold which, according to Jason, doesn't holds true for G1GC. Here is a list of where I could spot this:

@jrodewig
Copy link
Contributor

jrodewig commented Dec 2, 2020

Thanks @tsouza. I've created an issue to address this in our cloud docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrodewig jrodewig jrodewig approved these changes

@danielmitterdorfer danielmitterdorfer Awaiting requested review from danielmitterdorfer

@dliappis dliappis Awaiting requested review from dliappis

Assignees
No one assigned
Labels
:Core/Infra/Settings Settings infrastructure and APIs >docs General docs changes v7.10.2 v7.11.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ebadyano @elasticmachine @kostasb @jrodewig @tsouza @dliappis @jakelandis