Simplify number of shards setting#30783
Merged
jasontedor merged 1 commit intoelastic:masterfrom May 22, 2018
Merged
Conversation
This is code that was leftover from the move to one shard by default. Here in index metadata we were preserving the default number of shards settings independently of the area of code where we set this value on an index that does not explicitly have an number of shards setting. This took into consideration the es.index.max_number_of_shards system property, and was used in search requests to set the default maximum number of concurrent shard requests. We set the default there based on the default number of shards so that in a one-node case a search request could concurrently hit all shards on an index with the defaults. Now that we default to one shard, we expect fewer shards in clusters and this adjustment of the node count as the max number of concurrent shard requests is no longer needed. This commit then changes the default number of shards settings to be consistent with the value used when an index is created, and removes the now unneeded adjustment in search requests.
Collaborator
|
Pinging @elastic/es-core-infra |
jimczi
approved these changes
May 22, 2018
Contributor
jimczi
left a comment
There was a problem hiding this comment.
LGTM, I thought at first that we should keep a small constant to compute the default for max_concurrent_shard_request but I understand now that the intent was not to increase the concurrency but rather to hit all shards in a default index so +1 for the change.
| * it sane. A single search request that fans out to lots of shards should not hit a cluster too hard while 256 is already a | ||
| * lot. | ||
| */ | ||
| searchRequest.setMaxConcurrentShardRequests(Math.min(256, nodeCount)); |
Contributor
There was a problem hiding this comment.
+1, we could multiply nodeCount with a small constant to favor latency over throughput but the reasoning for the default value is to make sure that we hit all shards in a default index even when there is a single node so this change is consistent with the new default number of shards.
s1monw
approved these changes
May 22, 2018
Member
Author
dnhatn
added a commit
that referenced
this pull request
May 22, 2018
* master: QA: Add xpack tests to rolling upgrade (#30795) Modify state of VerifyRepositoryResponse for bwc (#30762) Reduce CLI scripts to one-liners on Windows (#30772) Simplify number of shards setting (#30783) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) [Docs] Fix script-fields snippet execution (#30693) Upgrade to Lucene-7.4.0-snapshot-cc2ee23050 (#30778) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) Make http pipelining support mandatory (#30695) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) Increase the maximum number of filters that may be in the cache. (#30655) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Add more yaml tests for get alias API (#29513) Ignore empty completion input (#30713) [DOCS] fixed incorrect default [ML] Filter undefined job groups from update calendar actions (#30757) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Enable installing plugins from snapshots.elastic.co (#30765) Remove fedora 26, add 28 (#30683) Accept Gradle build scan agreement (#30645) Remove logging from elasticsearch-nio jar (#30761) Add Delete Repository High Level REST API (#30666)
s1monw
added a commit
to s1monw/elasticsearch
that referenced
this pull request
Jun 7, 2018
We moved to 1 shard by default which caused some issues in how many concurrent shard requests we allow by default. For instance searching a 5 shard index on a single node will now be executed serially per shard while we want these cases to have a good concurrency out of the box. This change moves to defaults based on the avg. number of shards per index in the current search request to provide a good out of the box concurrency. Relates to elastic#30783 Closes elastic#30994
s1monw
added a commit
that referenced
this pull request
Jun 8, 2018
We moved to 1 shard by default which caused some issues in how many concurrent shard requests we allow by default. For instance searching a 5 shard index on a single node will now be executed serially per shard while we want these cases to have a good concurrency out of the box. This change moves to `numNodes * 5` which corresponds to the default we used to have in the previous version. Relates to #30783 Closes #30994
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is code that was leftover from the move to one shard by default. Here in index metadata we were preserving the default number of shards settings independently of the area of code where we set this value on an index that does not explicitly have an number of shards setting. This took into consideration the es.index.max_number_of_shards system property, and was used in search requests to set the default maximum number of concurrent shard requests. We set the default there based on the default number of shards so that in a one-node case a search request could concurrently hit all shards on an index with the defaults. Now that we default to one shard, we expect fewer shards in clusters and this adjustment of the node count as the max number of concurrent shard requests is no longer needed. This commit then changes the default number of shards settings to be consistent with the value used when an index is created, and removes the now unneeded adjustment in search requests.
Relates #30539