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

Refine size-your-shards wording #89081

Conversation

DaveCTurner
Copy link
Contributor

Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.

Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
@DaveCTurner DaveCTurner added >docs General docs changes :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v8.2.4 v8.3.4 v8.4.1 v8.5.0 labels Aug 3, 2022
@elasticsearchmachine elasticsearchmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Docs Meta label for docs team labels Aug 3, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@DaveCTurner
Copy link
Contributor Author

Copy link
Contributor

@jakommo jakommo left a comment

Choose a reason for hiding this comment

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

Thanks a lot @DaveCTurner ❤️ This makes it much more clear now !

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

Looks good though I have a few more wishes.

@@ -175,17 +175,24 @@ index prirep shard store

[discrete]
[[shard-count-recommendation]]
==== Aim for 3000 indices or fewer per GB of heap memory on each master node
==== Aim for fewer than 3000 indices per GB of heap memory on each master node
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should reverse the sentence here to say

Aim for 1 GB of heap memory on each master per 3000 indices

It is a recommendation for handling the necessary amount of indices, not a recommendation for how many indices they should have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer this order for a couple of reasons:

  1. IMO it's more common that the user knows the size of their nodes and is trying to work out a suitable sharding and retention strategy to match. For instance on Cloud there's only a few different possible master heap sizes.

  2. We want to phrase this as a bound, not a target, and I find it less natural to say "aim for more than 1GB of heap per 3000 indices".

Copy link
Contributor

Choose a reason for hiding this comment

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

About 1, this seems to be the wrong way around and I'd like to encourage that in the heading.

About 2, we can perhaps call it:

Suggested change
==== Aim for fewer than 3000 indices per GB of heap memory on each master node
==== Masters should have at least 1 GB of heap per 3000 indices.

each dedicated master node should have at least 4GB of heap. For non-dedicated
master nodes, the same rule holds and should be added to the heap requirements
of the other roles of each node.
As a general rule of thumb, you should have fewer than 3000 indices per GB of
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also reverse the sentence here to derive the GB from number of indices?

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM though I'd still like the heading change. Added a suggestion, but do not want to hold back the other additional text.

@@ -175,17 +175,24 @@ index prirep shard store

[discrete]
[[shard-count-recommendation]]
==== Aim for 3000 indices or fewer per GB of heap memory on each master node
==== Aim for fewer than 3000 indices per GB of heap memory on each master node
Copy link
Contributor

Choose a reason for hiding this comment

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

About 1, this seems to be the wrong way around and I'd like to encourage that in the heading.

About 2, we can perhaps call it:

Suggested change
==== Aim for fewer than 3000 indices per GB of heap memory on each master node
==== Masters should have at least 1 GB of heap per 3000 indices.

@DaveCTurner DaveCTurner added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge labels Aug 8, 2022
@elasticsearchmachine elasticsearchmachine merged commit c81f907 into elastic:main Aug 8, 2022
@DaveCTurner DaveCTurner deleted the 2022-08-03-size-your-shards-wording branch August 8, 2022 09:06
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.2
8.3
8.4

elasticsearchmachine pushed a commit that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
elasticsearchmachine pushed a commit that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
elasticsearchmachine pushed a commit that referenced this pull request Aug 8, 2022
Clarify that the limits in the docs are absolute maxima that will avoid
things just breaking but won't necessarily give great performance.
@mark-vieira mark-vieira added v8.4.0 and removed v8.4.1 labels Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >docs General docs changes Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Docs Meta label for docs team v8.2.4 v8.3.4 v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants